Skip to content

Report isolate as thread #847

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

Merged
merged 23 commits into from
Jun 7, 2022
Merged

Conversation

ueman
Copy link
Collaborator

@ueman ueman commented Apr 25, 2022

📜 Description

This PR introduces reporting of the current isolate as thread.

See for example events:

Event with isolate as thread

💡 Motivation and Context

Closes #839

💚 How did you test it?

Added a new test case.

📝 Checklist

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

🔮 Next steps

@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2022

Codecov Report

Merging #847 (32fc922) into main (9bedb1b) will increase coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #847      +/-   ##
==========================================
+ Coverage   89.76%   89.94%   +0.17%     
==========================================
  Files         102        9      -93     
  Lines        3146      169    -2977     
==========================================
- Hits         2824      152    -2672     
+ Misses        322       17     -305     
Impacted Files Coverage Δ
dart/lib/src/sentry_client.dart
dart/lib/src/sentry_exception_factory.dart
dart/lib/src/sentry_options.dart
dart/lib/src/protocol/mechanism.dart
dart/lib/src/client_reports/discarded_event.dart
...lib/src/client_reports/client_report_recorder.dart
dart/lib/src/protocol/sentry_thread.dart
dart/lib/src/throwable_mechanism.dart
dart/lib/src/protocol/span_status.dart
dart/lib/src/protocol/debug_image.dart
... and 83 more

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 9bedb1b...32fc922. Read the comment docs.

@ueman ueman marked this pull request as ready for review April 25, 2022 19:42
@marandaneto marandaneto marked this pull request as draft April 26, 2022 09:51
@marandaneto
Copy link
Contributor

Made it a draft since its blocked by #841

@ueman ueman marked this pull request as ready for review May 3, 2022 14:57
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@ueman ueman requested a review from marandaneto May 10, 2022 06:54
@marandaneto
Copy link
Contributor

@ueman please merge main into this branch, so I can test both changes together :) thanks.

@ueman
Copy link
Collaborator Author

ueman commented May 10, 2022

@ueman please merge main into this branch, so I can test both changes together :) thanks.

@marandaneto done

@ueman
Copy link
Collaborator Author

ueman commented May 10, 2022

@marandaneto When testing with the Flutter example app, you may need to enable the attachThreads option.

@ueman ueman requested a review from brustolin May 12, 2022 07:51
@marandaneto
Copy link
Contributor

@ueman not sure if its a bug on the Android SDK or this SDK, but if I capture an exception on the Dart side:

Screenshot 2022-05-13 at 12 40 37

current=false should be true.
I'm using options.attachThreads = true

@ueman
Copy link
Collaborator Author

ueman commented May 13, 2022

@ueman not sure if its a bug on the Android SDK or this SDK, but if I capture an exception on the Dart side:

Screenshot 2022-05-13 at 12 40 37

current=false should be true. I'm using options.attachThreads = true

I think this might be an error on the Android side. I even wrote tests, that make sure that current = true.
current is also never set to false as seen here:
https://github.com/ueman/sentry-dart/blob/40072f42a35c41deab011e25a54cbc642774c505/dart/lib/src/sentry_client.dart#L169-L176

and here

https://github.com/ueman/sentry-dart/blob/40072f42a35c41deab011e25a54cbc642774c505/dart/lib/src/sentry_client.dart#L198-L204

@marandaneto
Copy link
Contributor

marandaneto commented May 13, 2022

@ueman I can confirm, since it's correct on iOS.
getsentry/sentry-java#2047

@ueman
Copy link
Collaborator Author

ueman commented May 13, 2022

@ueman I can confirm, since it's correct on iOS. getsentry/sentry-java#2047

Soooo, this can be merged then, right?

@ueman ueman mentioned this pull request May 25, 2022
5 tasks
@ueman
Copy link
Collaborator Author

ueman commented May 25, 2022

Since the Sentry Android SDK was updated, this should now also work as expected on Android. The failing CI test is unrelated to my changes, since I didn't touch any iOS code.

@ueman
Copy link
Collaborator Author

ueman commented May 25, 2022

I've attached two example events to the PR description. It works really well now.
Also enabled that option for the example app.

For convenience:

@marandaneto
Copy link
Contributor

I've attached two example events to the PR description. It works really well now. Also enabled that option for the example app.

For convenience:

Thanks @ueman
We are releasing the Android SDK v6 right now and will test it soon.

@marandaneto marandaneto merged commit e754f11 into getsentry:main Jun 7, 2022
@ueman ueman deleted the feature/isolate-as-thread branch June 7, 2022 15:22
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.

Report isolate name and Android thread name in Thread context
4 participants