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

feat: add fragment lifecycle breadcrumb logger #1522

Merged
merged 18 commits into from
Jun 11, 2021
Merged

Conversation

wzieba
Copy link
Contributor

@wzieba wzieba commented Jun 10, 2021

📜 Description

This PR adds a new module sentry-android-fragment which exposes a new Integration for automated Android's Fragments lifecycles breadcrumb logging.

Sample

image

💡 Motivation and Context

Closes: #1374

From suggested solutions by @marandaneto , I've liked

Maybe installing a new ActivityFragmentLifecycleIntegration that relies on androidx.fragment.app and then calling activity.getSupportFragmentManager(), to add and remove the FragmentLifecycleIntegration callback.
this would likely require a new package as the SDK does not depend on the fragments package, like sentry-android-fragment module.

the most, so I've followed it in this implementation.

It's also highly inspired by ActivityLifecycleIntegration.

💚 How did you test it?

Aside from unit tests, I've tested it manually using a sample project. To do this, uncomment manual initialization in MyApplication, add a test DSN, run app and click on Open sample fragment button.

From there you can send a message from inner fragment to check if breadcrumbs logging works for nested fragments too.

📝 Checklist

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

🔮 Next steps

@marandaneto
Copy link
Contributor

@wzieba nicely done, thanks a lot, left a few comments only the Java or Kotlin would be a question to be answered, all the rest are easy fixes.
This integration will likely grow in features, eg creating spans (performance monitoring) automatically for every fragment

@wzieba
Copy link
Contributor Author

wzieba commented Jun 10, 2021

nicely done, thanks a lot, left a few comments only the Java or Kotlin would be a question to be answered, all the rest are easy fixes.

@marandaneto thank you a lot for the review!

I'm now fixing formatting issues - I couldn't reproduce them locally as I was using Java 11.0 on my machine. After changing JDK to 1.8 (the same as on CI) I was able to reproduce&fix them.

This integration will likely grow in features, eg creating spans (performance monitoring) automatically for every fragment

Sure, that's a good idea to add spans for this integration too.

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2021

Codecov Report

Merging #1522 (9910d77) into main (97c6a66) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1522   +/-   ##
=========================================
  Coverage     76.12%   76.12%           
  Complexity     1934     1934           
=========================================
  Files           191      191           
  Lines          6696     6696           
  Branches        666      666           
=========================================
  Hits           5097     5097           
  Misses         1274     1274           
  Partials        325      325           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97c6a66...9910d77. Read the comment docs.

@wzieba wzieba marked this pull request as ready for review June 10, 2021 17:12
@wzieba wzieba requested a review from marandaneto June 10, 2021 17:12
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

wow this is beautiful stuff. Thanks @wzieba

buildSrc/src/main/java/Config.kt Show resolved Hide resolved
Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

@wzieba good stuff, thanks a lot, also pretty nice the comments linking the commit hash, so easy to review, nicely done.
left 2 small comments, other than that LGTM.
I
l'll cut a release early next week :)

@wzieba
Copy link
Contributor Author

wzieba commented Jun 11, 2021

Thank you @marandaneto , it was a pleasure working on this. I've left one comment about parameterless constructor - please take a look and let me know if it needs changes.

l'll cut a release early next week :)

Great, thanks!

@marandaneto marandaneto merged commit 7cde581 into getsentry:main Jun 11, 2021
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.

Provide default fragment breadcrumb implementation
4 participants