Skip to content

Comments

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

Merged
romtsn merged 13 commits intofeat/observe-networkfrom
feat/rate-limit-cached-envelopes
Sep 28, 2023
Merged

Don't send cached envelopes when rate-limiting is active#2937
romtsn merged 13 commits intofeat/observe-networkfrom
feat/rate-limit-cached-envelopes

Conversation

@markushi
Copy link
Member

@markushi markushi commented Sep 14, 2023 •
edited by romtsn
Loading

📜 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 •
edited
Loading

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 •
edited
Loading

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

romtsn
romtsn approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@romtsn romtsn left a comment •
edited
Loading

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
stefanosiano
stefanosiano reviewed Sep 28, 2023
View reviewed changes
@codecov
Copy link

codecov bot commented Sep 28, 2023 •
edited
Loading

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
@romtsn romtsn deleted the feat/rate-limit-cached-envelopes branch September 28, 2023 13:20
@romtsn romtsn mentioned this pull request Sep 28, 2023
Closed
@romtsn romtsn mentioned this pull request Dec 27, 2023
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adinauer adinauer adinauer left review comments

@romtsn romtsn romtsn approved these changes

+1 more reviewer

@stefanosiano stefanosiano stefanosiano approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@romtsn romtsn

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@markushi @adinauer @romtsn @stefanosiano @getsentry-bot