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

Handle backpressure earlier in pipeline #2371

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Oct 23, 2024

📜 Description

Move the task queue where we limit the number of consecutive un-awaited SDK tasks to the earliest possible point in the SDK. That way we don't run event processors on events/transactions that will get dropped eventually.

💡 Motivation and Context

Relates to #2360
Relates to #2368

💚 How did you test it?

CI

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@buenaflor I think we should also keep #2368, as it is debouncing based on time, and the task queue is about how many parallel tasks can be scheduled with sentry, independent from their duration. WDYT?

Copy link

codecov bot commented Oct 23, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.77%. Comparing base (8f95e33) to head (05bc7c5).

Files with missing lines Patch % Lines
dart/lib/src/transport/task_queue.dart 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2371      +/-   ##
==========================================
- Coverage   84.78%   84.77%   -0.01%     
==========================================
  Files         253      253              
  Lines        9070     9080      +10     
==========================================
+ Hits         7690     7698       +8     
- Misses       1380     1382       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@denrase denrase marked this pull request as ready for review October 23, 2024 09:52
Copy link
Contributor

github-actions bot commented Oct 23, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 440.45 ms 477.56 ms 37.11 ms
Size 6.49 MiB 7.57 MiB 1.08 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
117d988 376.32 ms 450.85 ms 74.53 ms
b98109e 296.46 ms 362.68 ms 66.22 ms
90a08ea 477.25 ms 534.10 ms 56.85 ms
40680d3 323.55 ms 390.29 ms 66.73 ms
deaeece 347.42 ms 381.10 ms 33.68 ms
95d0636 301.46 ms 357.98 ms 56.52 ms
24b6e60 440.64 ms 557.96 ms 117.32 ms
c328ffc 394.35 ms 480.94 ms 86.59 ms
26e955b 369.52 ms 458.60 ms 89.07 ms
8d64376 302.88 ms 356.84 ms 53.96 ms

App size

Revision Plain With Sentry Diff
117d988 6.33 MiB 7.26 MiB 947.03 KiB
b98109e 6.06 MiB 7.03 MiB 993.53 KiB
90a08ea 6.49 MiB 7.55 MiB 1.06 MiB
40680d3 6.06 MiB 7.03 MiB 989.25 KiB
deaeece 5.94 MiB 6.96 MiB 1.02 MiB
95d0636 6.16 MiB 7.14 MiB 1007.32 KiB
24b6e60 6.33 MiB 7.26 MiB 950.14 KiB
c328ffc 6.35 MiB 7.42 MiB 1.07 MiB
26e955b 6.27 MiB 7.20 MiB 956.49 KiB
8d64376 5.94 MiB 6.96 MiB 1.02 MiB

Previous results on branch: enha/earlier-task-queue

Startup times

Revision Plain With Sentry Diff
42d6616 454.80 ms 486.77 ms 31.97 ms

App size

Revision Plain With Sentry Diff
42d6616 6.49 MiB 7.57 MiB 1.08 MiB

Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

@denrase I'm seeing this only now but we don't handle client reports here do we?

We have a type for that in the develop docs: queue_overflow: a SDK internal queue (eg: transport queue) overflowed

Copy link
Contributor

github-actions bot commented Oct 23, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1216.98 ms 1237.06 ms 20.08 ms
Size 8.38 MiB 9.75 MiB 1.37 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
a7acb24 1296.71 ms 1317.69 ms 20.98 ms
1e094d3 1237.54 ms 1243.51 ms 5.97 ms
ca9f398 1227.29 ms 1236.20 ms 8.92 ms
6f3717a 1259.84 ms 1280.90 ms 21.06 ms
0118295 1211.31 ms 1227.02 ms 15.71 ms
2e1e4ae 1254.41 ms 1278.55 ms 24.14 ms
136c365 1248.12 ms 1277.33 ms 29.20 ms
689d2fd 1257.71 ms 1265.16 ms 7.45 ms
d7758e8 1271.69 ms 1288.08 ms 16.39 ms
d61cecf 1276.96 ms 1290.02 ms 13.06 ms

App size

Revision Plain With Sentry Diff
a7acb24 8.16 MiB 9.17 MiB 1.01 MiB
1e094d3 8.29 MiB 9.37 MiB 1.08 MiB
ca9f398 8.38 MiB 9.74 MiB 1.36 MiB
6f3717a 8.33 MiB 9.62 MiB 1.29 MiB
0118295 8.32 MiB 9.38 MiB 1.05 MiB
2e1e4ae 8.34 MiB 9.65 MiB 1.31 MiB
136c365 8.38 MiB 9.75 MiB 1.37 MiB
689d2fd 8.10 MiB 9.16 MiB 1.06 MiB
d7758e8 8.15 MiB 9.12 MiB 989.76 KiB
d61cecf 8.10 MiB 9.18 MiB 1.08 MiB

Previous results on branch: enha/earlier-task-queue

Startup times

Revision Plain With Sentry Diff
42d6616 1247.06 ms 1255.36 ms 8.30 ms
05bc7c5 1241.57 ms 1271.11 ms 29.54 ms

App size

Revision Plain With Sentry Diff
42d6616 8.38 MiB 9.75 MiB 1.37 MiB
05bc7c5 8.38 MiB 9.75 MiB 1.37 MiB

@buenaflor
Copy link
Contributor

buenaflor commented Oct 23, 2024

I think we should also keep #2368, as it is debouncing based on time, and the task queue is about how many parallel tasks can be scheduled with sentry, independent from their duration. WDYT?

I just wanna make sure that the screenshot is still attached to the events that do go through the task queue, other than that I'm not against leaving the debouncing

I think it's also not a good experience if the screenshot is 'inconsistent', some errors have it, some don't

@denrase
Copy link
Collaborator Author

denrase commented Oct 23, 2024

I think it's also not a good experience if the screenshot is 'inconsistent', some errors have it, some don't

Fair point, this would indeed be an issue and potentially confusing. Also users can change the SDK behaviour by setting a different maxQueue value. Lets close the other PR then. 👍

@denrase denrase mentioned this pull request Oct 23, 2024
6 tasks
@@ -5,7 +5,10 @@
### Enhancements

- Cache parsed DSN ([#2365](https:/getsentry/sentry-dart/pull/2365))

- Handle backpressure earlier in pipeline ([#2371](https:/getsentry/sentry-dart/pull/2371))
- Drops max un-awaited parallel tasks earlier, so event processors & callbacks are not executed for them.
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens though if you await Sentry.captureException in a tight loop 100 times. Will those still be dropped? Since you mention here it only works for un-awaited parallel tasks

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.

2 participants