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

Various fixes to instrumentations running on the main thread #4039

Closed
wants to merge 11 commits into from

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Jan 10, 2025

📜 Description

Here are just some preventive measure that will make less code/instructions to run on the main thread:

  • Safety check for the Window.Callback to not be a SentryWindowCallback already to prevent multiple callbacks running simultaneously
  • Get rid of requireNonNull calls as they are redundant (we do instanceof checks later on anyway)
  • Check if tracing is enabled before instrumenting File I/O streams, because otherwise they go into the void anyway
  • Speed up ViewUtils.findTarget method by avoiding ViewGroup children traversal if the touch was not within the ViewGroup bounds

Before

Screenshot 2025-01-13 at 23 36 38

After

Screenshot 2025-01-14 at 00 09 42

💡 Motivation and Context

Internal customer discussions

💚 How did you test it?

automated

📝 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 Jan 10, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 398.04 ms 459.90 ms 61.85 ms
Size 1.65 MiB 2.31 MiB 677.52 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
00c8eeb 415.32 ms 428.08 ms 12.76 ms

App size

Revision Plain With Sentry Diff
00c8eeb 1.70 MiB 2.36 MiB 671.98 KiB

Previous results on branch: rz/fix/user-interaction-tracing

Startup times

Revision Plain With Sentry Diff
add3e5a 449.39 ms 481.34 ms 31.95 ms
4e86894 429.12 ms 479.11 ms 49.99 ms
95322cb 410.15 ms 489.38 ms 79.23 ms

App size

Revision Plain With Sentry Diff
add3e5a 1.65 MiB 2.31 MiB 677.51 KiB
4e86894 1.65 MiB 2.31 MiB 677.48 KiB
95322cb 1.65 MiB 2.31 MiB 677.51 KiB

@@ -274,6 +276,15 @@ class SentryFileInputStreamTest {
assertEquals(false, fileIOSpan.data[SpanDataConvention.BLOCKED_MAIN_THREAD_KEY])
assertNull(fileIOSpan.data[SpanDataConvention.CALL_STACK_KEY])
}

@Test
fun `when tracing is disabled does not instrument the stream`() {
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 add the same test in SentryFileOutputStreamTest?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I can add one!

Copy link
Member

@stefanosiano stefanosiano left a comment

Choose a reason for hiding this comment

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

just small details, but nice

@romtsn
Copy link
Member Author

romtsn commented Jan 15, 2025

Superseded by #4051

@romtsn romtsn closed this Jan 15, 2025
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