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

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

Merged
merged 13 commits into from
Sep 28, 2023

Conversation

markushi
Copy link
Member

@markushi markushi commented Sep 14, 2023

๐Ÿ“œ Description

Expose RateLimiter via Hub to let SendCachedEnvelope integrations decide if they should send envelopes now or later.

๐Ÿ’ก Motivation and Context

Fixes #1926

๐Ÿ’š How did you test it?

๐Ÿ“ 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 14, 2023

Messages
๐Ÿ“– Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by ๐Ÿšซ dangerJS against 0c4ee39

@github-actions
Copy link
Contributor

github-actions bot commented Sep 14, 2023

Performance metrics ๐Ÿš€

ย  Plain With Sentry Diff
Startup time 416.00 ms 483.96 ms 67.96 ms
Size 1.72 MiB 2.26 MiB 548.73 KiB

Baseline results on branch: feat/observe-network

Startup times

Revision Plain With Sentry Diff
2cd49ea 315.61 ms 361.77 ms 46.16 ms
8b1b6fc 316.96 ms 365.45 ms 48.49 ms
658a82c 309.27 ms 365.35 ms 56.08 ms
70f6e88 335.72 ms 372.84 ms 37.12 ms
4881e6a 296.22 ms 334.90 ms 38.68 ms
25a760c 310.30 ms 351.38 ms 41.08 ms
8ba4611 320.46 ms 361.84 ms 41.38 ms
ebdd7ec 360.63 ms 396.29 ms 35.65 ms
a5f58d4 341.46 ms 390.80 ms 49.34 ms
e2339ee 392.50 ms 466.21 ms 73.71 ms

App size

Revision Plain With Sentry Diff
2cd49ea 1.72 MiB 2.26 MiB 548.94 KiB
8b1b6fc 1.72 MiB 2.26 MiB 548.34 KiB
658a82c 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
25a760c 1.72 MiB 2.26 MiB 549.33 KiB
8ba4611 1.72 MiB 2.26 MiB 548.94 KiB
ebdd7ec 1.72 MiB 2.26 MiB 548.22 KiB
a5f58d4 1.72 MiB 2.26 MiB 548.54 KiB
e2339ee 1.72 MiB 2.26 MiB 548.22 KiB

Previous results on branch: feat/rate-limit-cached-envelopes

Startup times

Revision Plain With Sentry Diff
02dfd7c 373.59 ms 399.64 ms 26.05 ms
e0756a9 367.21 ms 441.22 ms 74.01 ms
d3ab716 347.13 ms 429.68 ms 82.55 ms
194fa5c 333.24 ms 418.40 ms 85.15 ms
376470b 318.90 ms 362.02 ms 43.12 ms
b2aaa92 329.94 ms 375.72 ms 45.78 ms
3e70915 390.37 ms 483.65 ms 93.27 ms
8b0da8c 403.04 ms 478.96 ms 75.92 ms
de39dbc 377.41 ms 400.90 ms 23.49 ms

App size

Revision Plain With Sentry Diff
02dfd7c 1.72 MiB 2.26 MiB 548.74 KiB
e0756a9 1.72 MiB 2.26 MiB 548.71 KiB
d3ab716 1.72 MiB 2.26 MiB 548.93 KiB
194fa5c 1.72 MiB 2.26 MiB 548.71 KiB
376470b 1.72 MiB 2.26 MiB 548.40 KiB
b2aaa92 1.72 MiB 2.26 MiB 548.73 KiB
3e70915 1.72 MiB 2.26 MiB 548.71 KiB
8b0da8c 1.72 MiB 2.26 MiB 548.72 KiB
de39dbc 1.72 MiB 2.26 MiB 548.93 KiB

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.

Looking good, only one thing - maybe we should add the delay to avoid bursting our server with a bunch of events after a while and hitting another rate-limit? Similar to how it's done in https://github.com/getsentry/sentry-cocoa/pull/2263/files

@romtsn romtsn self-assigned this Sep 26, 2023
@romtsn romtsn marked this pull request as ready for review September 28, 2023 09:46
@romtsn romtsn force-pushed the feat/rate-limit-cached-envelopes branch from 58adccc to 4707bd3 Compare September 28, 2023 11:01
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

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

Files Coverage ฮ”
...try/android/core/ActivityLifecycleIntegration.java 83.38% <100.00%> (รธ)
...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%> (รธ)
...try/android/core/DefaultAndroidEventProcessor.java 84.68% <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%> (รธ)
... and 121 more

... and 63 files with indirect coverage changes

๐Ÿ“ข Thoughts on this report? Let us know!.

@romtsn romtsn merged commit d911aa8 into feat/observe-network Sep 28, 2023
15 checks passed
@romtsn romtsn deleted the feat/rate-limit-cached-envelopes branch September 28, 2023 13:20
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.

5 participants