Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CSR-2011] feat: allow reporting instance results in different requests #130

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

miguelangaranocurrents
Copy link
Contributor

@miguelangaranocurrents miguelangaranocurrents commented Jan 7, 2025

Description

Provide a brief description of the changes in this PR. Important to list all the changes made in overall. Describe any improvements, follow up tasks or edge cases related to this PR

This PR allows dividing the reporting of the instance results into multiple requests. Initially it is dividing the results if the array is more than 1MB in 1MB max parts.
The fullTestSuite is only part of the first createRun request.

IMPORTANT NOTE: While doing the tests with a really big amount of spec files (about 2k) and after some minutes after the run finished, I'm seeing this behavior:
image
image

This is much likely related to the redis OOM issue (CSR-1967). Checked with DJ this and it seems to be that, but we'll have to wait until those changes are merged and working to check again if this issue shows up again.

PR Checklist

  • I performed manual tests or added tests that prove my fix is effective or that my feature works.
  • I have performed a self-review of my code.
  • I have annotated my PR with comments, particularly in hard-to-understand areas.
  • I have considered the security implications of this work.

Release Plan

Do we need to update any environment variables? Is there any order of releases required?

  • Package release

Demo

Add screenshots or videos demonstrating the solution if applicable

https://www.loom.com/share/d547d24c6b16401a8178b5d8f5ad780a?sid=e03781a0-b548-4bc3-a980-c2884eb2d46e

Manual Testing

Describe in steps (support it with images if needed) how to try the changes of this PR. (Even the start/setup of the services needed to try it out). If it's a bug also include reproduction steps

  1. Generate an XML file with at least 2MB of tests
  2. Use the convert command to create the instance files
  3. Use the upload command to report the results

Copy link

Copy link
Collaborator

@vCaisim vCaisim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this approach could be refined further. While it seems to work locally, it's unclear what impact it might have in a production environment. We should have better control over the number of requests made to our server. Is it safe and efficient to call the run creation endpoint this frequently? Additionally, maybe explore the options like "payload compression" or "uploading the file to S3", or a combination of "multiple requests" with "payload compression" to optimize performance?

@miguelangaranocurrents
Copy link
Contributor Author

I believe this approach could be refined further. While it seems to work locally, it's unclear what impact it might have in a production environment. We should have better control over the number of requests made to our server. Is it safe and efficient to call the run creation endpoint this frequently? Additionally, maybe explore the options like "payload compression" or "uploading the file to S3", or a combination of "multiple requests" with "payload compression" to optimize performance?

Hi @vCaisim so currently the combination of multiple requests with payload compression is already in use. You can check the payload compression in the createRun function, line 96.

Regarding the redis issue, I just checked with DJ and mentioned that is probably because the already known issue with redis, we should wait until that is solved (CSR-1967) so we can see the redis error gone.

@agoldis
Copy link
Contributor

agoldis commented Jan 8, 2025

@miguelangaranocurrents @vCaisim Please do some testing on staging to estimate the expected load and performance before implementing S3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants