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 ignored check performance #3992

Merged
merged 13 commits into from
Dec 20, 2024
Merged

Conversation

lbloder
Copy link
Collaborator

@lbloder lbloder commented Dec 17, 2024

📜 Description

Improves performance of checking whether certain SpanOrigins, TransactionNames or Checkins should be ignored.

💡 Motivation and Context

Fixes #3967

  • Do equals check for all ignored items before regex check
  • Use precompiled Pattern for regex check
  • For SpanOrigins also add a decision cache

💚 How did you test it?

📝 Checklist

  • 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.
  • I updated the wizard 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

Copy link
Contributor

github-actions bot commented Dec 17, 2024

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

Generated by 🚫 dangerJS against 2b80ccd

Copy link
Contributor

github-actions bot commented Dec 17, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 390.73 ms 421.17 ms 30.43 ms
Size 1.65 MiB 2.31 MiB 676.54 KiB

Previous results on branch: feat/improve-ignored-check-performance

Startup times

Revision Plain With Sentry Diff
3e8377a 472.27 ms 499.42 ms 27.15 ms

App size

Revision Plain With Sentry Diff
3e8377a 1.65 MiB 2.31 MiB 676.54 KiB

Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

Mostly LGTM already. Can you please add your performance comparison results?

sentry/src/main/java/io/sentry/FilterString.java Outdated Show resolved Hide resolved
import kotlin.test.assertFalse
import kotlin.test.assertTrue

class SpanUtilsTest {
Copy link
Member

Choose a reason for hiding this comment

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

Should we test asking for the same origin again, so we can also test the cache?

Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>
@lbloder lbloder marked this pull request as ready for review December 20, 2024 12:49
@lbloder lbloder merged commit 35af39a into 8.x.x Dec 20, 2024
2 checks passed
@lbloder lbloder deleted the feat/improve-ignored-check-performance branch December 20, 2024 13:47
@lbloder
Copy link
Collaborator Author

lbloder commented Dec 20, 2024

jmh benchmarks run on Java 17 (17.0.4.1 Temurin):

Benchmarks comparing the changes with our current implementation:
All Tests are run with 5 clean starts with 5 warmup runs and 5 actual benchmarking runs resulting in 25 benchmark runs per test. Each benchmark is run for 10 seconds and records the throughput achieved during its runtime.

Each implementation is given a list of 10 identical origins to be ignored.
For the original implementation this is a list of 10 strings, for the new implementation a list of 10 Strings with precompiled regex patterns.
The tests were conducted without the additional cache that was implemented in SpanUtils.

Comparison when no origin matches

All 10 items in the list need to be checked.

Benchmark Mode Cnt Score   Error Units Result
originalFilterNoMatches thrpt 25 790229,812 ± 5339,308 ops/s 100%
newFilterNoMatches thrpt 25 2992883037,803 ± 81483766,098 ops/s 378736%

Comparison String equals

The 6th entry in the list matches using String.equals.

Benchmark Mode Cnt Score   Error Units Result
originalFilterEqualMatches thrpt 25 1573549,139 ± 38803,010 ops/s 100%
newFilterEqualsMatches thrpt 25 3024577381,724 ± 41660875,145 ops/s 192214%

Comparison regex match

The 7th entry in the list matches using regex pattern.

Benchmark Mode Cnt Score   Error Units Result
originalFilterPatternMatches thrpt 25 1020669,472 ± 5584,216 ops/s 100%
newFilterPatternMatches thrpt 25 3024226052,507 ± 123600352,187 ops/s 296298%

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.

3 participants