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

improve default debouncing mechanism #2945

Merged
merged 9 commits into from
Sep 29, 2023

Conversation

stefanosiano
Copy link
Member

📜 Description

improve default SS/VH debouncing mechanism: after 3 times occurring in a time frame, the next occurrence is debounced
added execution counter to Debouncer

💡 Motivation and Context

Closes #2917

💚 How did you test it?

Unit test

📝 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

added execution counter to Debouncer
@github-actions
Copy link
Contributor

github-actions bot commented Sep 20, 2023

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

Generated by 🚫 dangerJS against bd028eb

@stefanosiano
Copy link
Member Author

There is also another way: debounce randomly after the first event, with a hard limit of debouncing in a specific time frame.
In other words, allow SS/VH (and increase the counter) randomly after the first event, and not on every event.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 20, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 381.80 ms 452.68 ms 70.88 ms
Size 1.72 MiB 2.29 MiB 576.34 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
9e60fc1 283.04 ms 324.09 ms 41.04 ms
496bdfd 272.86 ms 407.33 ms 134.48 ms
9e60fc1 310.37 ms 359.48 ms 49.11 ms
496bdfd 301.22 ms 343.96 ms 42.73 ms
87b3774 310.48 ms 362.04 ms 51.56 ms
9e60fc1 334.08 ms 380.00 ms 45.92 ms
4bf202b 331.20 ms 345.24 ms 14.04 ms
9e60fc1 337.84 ms 385.24 ms 47.40 ms
adf8fe3 300.49 ms 357.36 ms 56.87 ms
9e60fc1 308.22 ms 361.72 ms 53.50 ms

App size

Revision Plain With Sentry Diff
9e60fc1 1.72 MiB 2.29 MiB 575.91 KiB
496bdfd 1.72 MiB 2.28 MiB 571.82 KiB
9e60fc1 1.72 MiB 2.29 MiB 575.91 KiB
496bdfd 1.72 MiB 2.28 MiB 571.82 KiB
87b3774 1.72 MiB 2.29 MiB 575.54 KiB
9e60fc1 1.72 MiB 2.29 MiB 575.91 KiB
4bf202b 1.72 MiB 2.29 MiB 575.54 KiB
9e60fc1 1.72 MiB 2.29 MiB 575.91 KiB
adf8fe3 1.72 MiB 2.29 MiB 575.24 KiB
9e60fc1 1.72 MiB 2.29 MiB 575.91 KiB

Previous results on branch: feat/improve-default-debouncing

Startup times

Revision Plain With Sentry Diff
a9bde15 370.10 ms 441.64 ms 71.54 ms
06c1ec2 361.11 ms 456.84 ms 95.73 ms
400216b 382.32 ms 446.04 ms 63.72 ms
83de545 318.80 ms 370.10 ms 51.30 ms
6d1a864 362.58 ms 439.10 ms 76.52 ms

App size

Revision Plain With Sentry Diff
a9bde15 1.72 MiB 2.29 MiB 576.02 KiB
06c1ec2 1.72 MiB 2.29 MiB 576.33 KiB
400216b 1.72 MiB 2.29 MiB 576.02 KiB
83de545 1.72 MiB 2.29 MiB 576.02 KiB
6d1a864 1.72 MiB 2.29 MiB 576.02 KiB

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

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

Files Coverage Δ
.../sentry/android/core/ScreenshotEventProcessor.java 96.96% <100.00%> (+0.09%) ⬆️
...ntry/android/core/ViewHierarchyEventProcessor.java 81.30% <100.00%> (+0.15%) ⬆️
...o/sentry/android/core/internal/util/Debouncer.java 100.00% <100.00%> (ø)
sentry/src/main/java/io/sentry/CheckInStatus.java 100.00% <100.00%> (ø)
sentry/src/main/java/io/sentry/Hub.java 75.84% <100.00%> (+0.67%) ⬆️
sentry/src/main/java/io/sentry/HubAdapter.java 95.65% <100.00%> (+0.06%) ⬆️
sentry/src/main/java/io/sentry/IHub.java 92.59% <ø> (ø)
sentry/src/main/java/io/sentry/ISentryClient.java 95.45% <ø> (ø)
...y/src/main/java/io/sentry/MonitorScheduleType.java 100.00% <100.00%> (ø)
...y/src/main/java/io/sentry/MonitorScheduleUnit.java 100.00% <100.00%> (ø)
... and 10 more

... and 17 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@stefanosiano
Copy link
Member Author

There is also another way: debounce randomly after the first event, with a hard limit of debouncing in a specific time frame. In other words, allow SS/VH (and increase the counter) randomly after the first event, and not on every event.

Thinking more about it, having random number would make it possible to have events with View Hierarchy, but without Screeshot and viceversa. Probably good enough as is

@stefanosiano stefanosiano marked this pull request as ready for review September 25, 2023 07:56
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.

LGTM, besides one comment wrt concurrency

@stefanosiano stefanosiano merged commit 1a21cf4 into main Sep 29, 2023
19 of 21 checks passed
@stefanosiano stefanosiano deleted the feat/improve-default-debouncing branch September 29, 2023 13:13
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.

Improve Screenshot/ViewHierarchy debouncing default behavior
3 participants