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

Fix regression on default sending behavior of the HttpTransport transport #905

Merged
merged 3 commits into from
Oct 18, 2019
Merged

Fix regression on default sending behavior of the HttpTransport transport #905

merged 3 commits into from
Oct 18, 2019

Conversation

ste93cry
Copy link
Contributor

@ste93cry ste93cry commented Oct 17, 2019

Q A
Branch? 2.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
License MIT

This PR closes #904 by fixing one regression introduced in #885. In particular, I accidentally changed the default behavior of the HttpTransport transport by sending events immediatly instead of waiting the shutdown of the application. This is a deprecated behavior, but the default must not change at least until 3.0 as it may have a huge impact on performances

@helhum
Copy link

helhum commented Oct 17, 2019

Perfect! That is exactly how I understood that it should be. Thanks a bunch for creating the PR and for helping me out understanding current and desired state!

@ste93cry
Copy link
Contributor Author

I'm not sure we actually want to consider returning null when an event fails to be sent a regression though. @HazAT do Unified API gives some hint on how this should be handled? Since the send method can return null values probably what you did is fine as-is. If that's the case I will revert the change

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

🚢

@ste93cry ste93cry merged commit 3ea3d3d into getsentry:master Oct 18, 2019
@ste93cry ste93cry deleted the fix/revert-sending-behavior-change-http-transport branch October 18, 2019 08:10
@Jean85 Jean85 mentioned this pull request Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asynchronous client not working correctly
6 participants