Observe network state to upload any unsent envelopes by markushi · Pull Request #2910 · getsentry/sentry-java

markushi · 2023-09-01T08:54:52Z

📜 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 · 2023-09-01T08:56:12Z

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

Generated by 🚫 dangerJS against 19cf91d

github-actions · 2023-09-01T09:11:23Z

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 · 2023-09-01T09:20:37Z

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)

…va into feat/observe-network

romtsn

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

...core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java

sentry/src/main/java/io/sentry/EnvelopeSender.java

sentry/src/main/java/io/sentry/transport/RateLimiter.java

sentry/src/main/java/io/sentry/cache/EnvelopeCache.java

adinauer

Gave this a quick look, LGTM.

sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java

...core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java

sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java

sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt

stefanosiano

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

Co-authored-by: Roman Zavarnitsyn <[email protected]> Co-authored-by: Sentry Github Bot <[email protected]>

codecov · 2023-09-28T14:07:34Z

Codecov Report

Attention: 442 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
.../main/java/io/sentry/android/core/ANRWatchDog.java	`76.92% <ø> (ø)`
...a/io/sentry/android/core/AndroidTransportGate.java	`100.00% <100.00%> (ø)`
...roid/core/AppComponentsBreadcrumbsIntegration.java	`89.06% <100.00%> (ø)`
...o/sentry/android/core/AppLifecycleIntegration.java	`68.25% <100.00%> (ø)`
...ain/java/io/sentry/android/core/AppStartState.java	`97.72% <100.00%> (ø)`
.../android/core/EnvelopeFileObserverIntegration.java	`83.33% <100.00%> (ø)`
...in/java/io/sentry/android/core/NdkIntegration.java	`91.83% <100.00%> (ø)`
...ry/android/core/NetworkBreadcrumbsIntegration.java	`93.93% <100.00%> (ø)`
...android/core/PhoneStateBreadcrumbsIntegration.java	`84.09% <100.00%> (ø)`
...a/io/sentry/android/core/SentryAndroidOptions.java	`92.43% <100.00%> (ø)`
... and 106 more

... and 105 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

dave-t-tran · 2023-11-08T13:27:50Z

@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 · 2023-11-08T13:51:15Z

@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

markushi added 3 commits August 31, 2023 16:59

Send cached envelopes only if there's a network connection

0b8c9c9

Add more tests and docs

f73ff15

Fix bad naming

eb98375

markushi requested review from adinauer, romtsn and stefanosiano as code owners September 1, 2023 08:54

markushi added 2 commits September 4, 2023 09:35

Add connection observer to SendCachedEnvelopeIntegration

cfb4bbd

Update Changelog

c8f099a

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

markushi and others added 13 commits September 4, 2023 10:27

Fix test

d088df7

Add more tests

d7fa34c

Cleanup disconnect handling, add logging

0e1c745

Add more tests

f2d89f9

Fix test

da8fc6d

Format code

b92c1f2

Merge branch 'feat/7.0.0' into feat/observe-network

d0b3881

Merge branch 'feat/observe-network' of github.com:getsentry/sentry-ja…

056e55b

…va into feat/observe-network

Merge branch 'feat/7.0.0' into feat/observe-network

d7cd79d

Merge branch 'feat/7.0.0' into feat/observe-network

7569d13

Ensure envelopes from cache are not sent multiple times

07bb07e

Fix api file

7dcff63

Fix tests

dcafe87

romtsn approved these changes Sep 19, 2023

View reviewed changes

adinauer approved these changes Sep 22, 2023

View reviewed changes

stefanosiano reviewed Sep 27, 2023

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java Outdated Show resolved Hide resolved

stefanosiano reviewed Sep 27, 2023

View reviewed changes

...core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java Show resolved Hide resolved

stefanosiano reviewed Sep 27, 2023

View reviewed changes

sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java Outdated Show resolved Hide resolved

stefanosiano reviewed Sep 27, 2023

View reviewed changes

sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt Outdated Show resolved Hide resolved

stefanosiano approved these changes Sep 27, 2023

View reviewed changes

markushi and others added 2 commits September 28, 2023 15:20

Don't send cached envelopes when rate-limiting is active (#2937)

d911aa8

Co-authored-by: Roman Zavarnitsyn <[email protected]> Co-authored-by: Sentry Github Bot <[email protected]>

Merge branch 'feat/7.0.0' into feat/observe-network

932b404

Address PR review

19cf91d

romtsn merged commit d6cd78c into feat/7.0.0 Sep 28, 2023

romtsn deleted the feat/observe-network branch September 28, 2023 15:29

Uh oh!

Comments

Conversation

markushi commented Sep 1, 2023

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions bot commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Previous results on branch: feat/observe-network

Startup times

App size

Uh oh!

romtsn commented Sep 1, 2023

Uh oh!

romtsn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adinauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefanosiano left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dave-t-tran commented Nov 8, 2023

Uh oh!

romtsn commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions bot commented Sep 1, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 28, 2023 •

edited

Loading