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

Observe network state to upload any unsent envelopes #2910

Merged
merged 21 commits into from
Sep 28, 2023

Conversation

markushi
Copy link
Member

@markushi markushi commented Sep 1, 2023

📜 Description

Adds a ConnectionStatusProvider mechanic, so we can observe changes to the connection state.
API wise this is already complete, only some more tests for SendCachedEnvelopeFireAndForgetIntegration are required.

💡 Motivation and Context

Partially implements #912

💚 How did you test it?

Added unit tests.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 19cf91d

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 378.55 ms 445.47 ms 66.92 ms
Size 1.72 MiB 2.26 MiB 550.13 KiB

Previous results on branch: feat/observe-network

Startup times

Revision Plain With Sentry Diff
ebdd7ec 360.63 ms 396.29 ms 35.65 ms
832584f 314.72 ms 362.38 ms 47.66 ms
25a760c 310.30 ms 351.38 ms 41.08 ms
658a82c 309.27 ms 365.35 ms 56.08 ms
8b1b6fc 316.96 ms 365.45 ms 48.49 ms
70f6e88 335.72 ms 372.84 ms 37.12 ms
4881e6a 296.22 ms 334.90 ms 38.68 ms
8a1f1f3 404.78 ms 486.32 ms 81.54 ms
2cd49ea 315.61 ms 361.77 ms 46.16 ms
8ba4611 320.46 ms 361.84 ms 41.38 ms

App size

Revision Plain With Sentry Diff
ebdd7ec 1.72 MiB 2.26 MiB 548.22 KiB
832584f 1.72 MiB 2.26 MiB 549.33 KiB
25a760c 1.72 MiB 2.26 MiB 549.33 KiB
658a82c 1.72 MiB 2.26 MiB 548.34 KiB
8b1b6fc 1.72 MiB 2.26 MiB 548.34 KiB
70f6e88 1.72 MiB 2.26 MiB 549.33 KiB
4881e6a 1.72 MiB 2.26 MiB 548.34 KiB
8a1f1f3 1.72 MiB 2.26 MiB 550.12 KiB
2cd49ea 1.72 MiB 2.26 MiB 548.94 KiB
8ba4611 1.72 MiB 2.26 MiB 548.94 KiB

@romtsn
Copy link
Member

romtsn commented Sep 1, 2023

Note that on Android we use SendCachedEnvelopeIntegration, so the change has likely to be implemented for both SendCachedEnvelopeIntegration and SendCachedEnvelopeFireAndForgetIntegration.

The former one is android-only as we also check for startup crashes there and block in case there are any. The latter one is jvm/backend only, and has to be manually installed by our users. We could also try to combine those two into one, but I remember I didn't want to do that, because on pure jvm apps we don't really need it, see #2277 (comment)

@markushi markushi changed the title Send cached envelopes only if there's a network connection Observe network state to upload any unsent envelopes Sep 4, 2023
Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

A few minor things:

  • Left some comments what I think make sense to change a bit
  • A few tests missing for the sentry module
  • A few final keywords missing

Otherwise looks great, I think this is a crucial improvement 🚀

CHANGELOG.md Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/EnvelopeSender.java Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/transport/RateLimiter.java Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/cache/EnvelopeCache.java Outdated Show resolved Hide resolved
Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

Gave this a quick look, LGTM.

Copy link
Member

@stefanosiano stefanosiano left a comment

Choose a reason for hiding this comment

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

looks great, just fix the comments left and we're good to go!

@codecov
Copy link

codecov bot commented Sep 28, 2023

@romtsn romtsn merged commit d6cd78c into feat/7.0.0 Sep 28, 2023
14 checks passed
@romtsn romtsn deleted the feat/observe-network branch September 28, 2023 15:29
@dave-t-tran
Copy link

@romtsn I noticed these code changes aren't in https://github.com/getsentry/sentry-java/releases/tag/7.0.0-rc.1. Are these changes slated for a future 7.x release?

@romtsn
Copy link
Member

romtsn commented Nov 8, 2023

@dave-t-tran they've been part of https://github.com/getsentry/sentry-java/releases/tag/7.0.0-beta.1 already, so the rc.1 also contains them

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.

6 participants