Skip to content

Overload withSentryObservableEffect to provide compose instrumentation hook#2320

Merged
markushi merged 7 commits intomainfrom
feature/compose-navigation-instrumentation
Oct 28, 2022
Merged

Overload withSentryObservableEffect to provide compose instrumentation hook#2320
markushi merged 7 commits intomainfrom
feature/compose-navigation-instrumentation

Conversation

@markushi
Copy link
Copy Markdown
Member

@markushi markushi commented Oct 24, 2022 •
edited
Loading

📜 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
Copy Markdown
Contributor

github-actions bot commented Oct 24, 2022 •
edited
Loading

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

Generated by 🚫 dangerJS against 4441c5f

@markushi markushi mentioned this pull request Oct 24, 2022
Merged
6 tasks
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 24, 2022 •
edited
Loading

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

romtsn
romtsn reviewed Oct 24, 2022
View reviewed changes
Comment thread sentry-compose/src/androidMain/kotlin/io/sentry/compose/SentryNavigationIntegration.kt Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 27, 2022 •
edited
Loading

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
romtsn reviewed Oct 27, 2022
View reviewed changes
Comment thread sentry-compose/src/androidMain/kotlin/io/sentry/compose/SentryNavigationIntegration.kt Outdated
@romtsn
Copy link
Copy Markdown
Member

romtsn commented Oct 27, 2022 •
edited
Loading

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

@markushi markushi mentioned this pull request Oct 28, 2022
Merged
1 task
@markushi markushi merged commit b516cf3 into main Oct 28, 2022
@markushi markushi deleted the feature/compose-navigation-instrumentation branch October 28, 2022 08:15
@gslevinkov gslevinkov mentioned this pull request Jan 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romtsn romtsn romtsn approved these changes

@adinauer adinauer Awaiting requested review from adinauer adinauer is a code owner

@stefanosiano stefanosiano Awaiting requested review from stefanosiano

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@markushi @codecov-commenter @romtsn