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

Report http response errors during upoad. #223

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Report http response errors during upoad. #223

merged 1 commit into from
Sep 17, 2024

Conversation

zhming0
Copy link
Contributor

@zhming0 zhming0 commented Sep 16, 2024

part of PIE-2813

Fix uploader so it will throw a runtime error if backend does not return 2xx.
This will result in a log message if any upload fails.

Note it will not fail a build if the upload fails.

@zhming0 zhming0 requested a review from a team September 16, 2024 06:09
@wooly
Copy link

wooly commented Sep 16, 2024

I'm not super keen on this, since it will cause people's builds to fail if the upload API is down. I always thought of the test collectors as a best-effort service.

What do you think about making this a configuration option so people could turn it on if they so wished?
How does this behaviour compare to the other collectors? Do they raise an issue if the data cannot be sent?

@zhming0
Copy link
Contributor Author

zhming0 commented Sep 16, 2024

I'm not super keen on this, since it will cause people's builds to fail if the upload API is down. I always thought of the test collectors as a best-effort service.

@wooly and @nprizal

It won't fail people's build. Our uploader has logic capturing the error.

It was just previously, the error reporting was never working, so we were completely in the dark about what happen in the collector.

Copy link
Contributor

@gchan gchan left a comment

Choose a reason for hiding this comment

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

lgtm, have we considered adding a tests to verify this new functionality? I assume you have manually tested this and viewed the new log message.

@wooly
Copy link

wooly commented Sep 16, 2024

@zhming0 ah yep I see it now, sorry for the noise!

@zhming0
Copy link
Contributor Author

zhming0 commented Sep 17, 2024

@gchan I added some tests. 👍🏿

@zhming0 zhming0 merged commit fbb2dec into main Sep 17, 2024
1 check passed
@zhming0 zhming0 deleted the ming/pie-2813 branch September 17, 2024 00:22
@zhming0 zhming0 changed the title Handle http response errors during upoad. Report http response errors during upoad. Sep 17, 2024
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