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

Overload withSentryObservableEffect to provide compose instrumentation hook #2320

Merged
merged 7 commits into from Oct 28, 2022

Conversation

markushi
Copy link
Member

@markushi markushi commented Oct 24, 2022

📜 Description

Overload withSentryObservableEffect to provide compose instrumentation hook

💡 Motivation and Context

In order to auto-instrument navigation in Jetpack Compose we simply want to adapt the original rememberNavController call with our sentry specific navigation listeners. This way the required bytecode changes can be kept to a minimum.

Related to #2319

💚 How did you test it?

Right now just manually, need to investigate how all this Android specifics can be tested.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2022

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

Generated by 🚫 dangerJS against 4441c5f

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 343.35 ms 378.78 ms 35.43 ms
Size 1.73 MiB 2.32 MiB 608.62 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
0fbe535 280.92 ms 373.04 ms 92.12 ms
0fbe535 396.74 ms 461.19 ms 64.45 ms
0fbe535 246.12 ms 288.64 ms 42.52 ms

App size

Revision Plain With Sentry Diff
0fbe535 1.73 MiB 2.32 MiB 608.62 KiB
0fbe535 1.73 MiB 2.32 MiB 608.62 KiB
0fbe535 1.73 MiB 2.32 MiB 608.62 KiB

Previous results on branch: feature/compose-navigation-instrumentation

Startup times

Revision Plain With Sentry Diff
7ae4fed 313.60 ms 370.04 ms 56.44 ms
a67bbd1 293.22 ms 329.08 ms 35.86 ms
275b116 244.33 ms 323.40 ms 79.07 ms
3dc8731 349.30 ms 408.34 ms 59.04 ms
6827509 291.26 ms 332.02 ms 40.76 ms

App size

Revision Plain With Sentry Diff
7ae4fed 1.73 MiB 2.32 MiB 608.62 KiB
a67bbd1 1.73 MiB 2.32 MiB 608.62 KiB
275b116 1.73 MiB 2.32 MiB 608.62 KiB
3dc8731 1.73 MiB 2.32 MiB 608.44 KiB
6827509 1.73 MiB 2.32 MiB 608.62 KiB

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2022

Codecov Report

Base: 80.21% // Head: 80.21% // No change to project coverage 👍

Coverage data is based on head (4441c5f) compared to base (0fbe535).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2320   +/-   ##
=========================================
  Coverage     80.21%   80.21%           
  Complexity     3475     3475           
=========================================
  Files           247      247           
  Lines         12906    12906           
  Branches       1735     1735           
=========================================
  Hits          10352    10352           
  Misses         1894     1894           
  Partials        660      660           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@markushi markushi changed the title Add rememberNavController to provide compose navigation hook Overload withSentryObservableEffect to provide compose instrumentation hoook Oct 27, 2022
@markushi markushi marked this pull request as ready for review October 27, 2022 07:13
@markushi markushi changed the title Overload withSentryObservableEffect to provide compose instrumentation hoook Overload withSentryObservableEffect to provide compose instrumentation hook Oct 27, 2022
@romtsn
Copy link
Member

romtsn commented Oct 27, 2022

one small remark, but otherwise lgtm, needs a changelog entry tho

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.

None yet

3 participants