Skip to content

Conversation

@denrase
Copy link
Collaborator

@denrase denrase commented Dec 13, 2022

📜 Description

Closes #456

Always set to unhandled, as there are no handlers provided:

  • IsolateErrorIntegration
  • RunZonedGuardedIntegration

Set to unhandled if there are no user provided handlers:

  • FlutterErrorIntegration
  • OnErrorIntegration

Verified in tests that handled is null on the mechanism. Technically those should also be unhandled, as users are not manually calling the Sentry SDK, but failed requests are something so common and should probably not be unhandled?

  • IoExceptionEventProcessor
  • FailedRequestClient
  • FailedRequestInterceptor

💚 How did you test it?

Added unit tests.

📝 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

Review every individually created mechanism if the handled flag makes sense.

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2022

Codecov Report

❗ No coverage uploaded for pull request base (v7.0.0@20def04). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##             v7.0.0    #1186   +/-   ##
=========================================
  Coverage          ?   76.27%           
=========================================
  Files             ?       11           
  Lines             ?      333           
  Branches          ?        0           
=========================================
  Hits              ?      254           
  Misses            ?       79           
  Partials          ?        0           

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.

@denrase denrase marked this pull request as ready for review December 13, 2022 10:26
@marandaneto
Copy link
Contributor

@denrase please check if the _eventWithRemovedBreadcrumbsIfHandled is not causing duplicated breadcrumbs as well because it depends on the handled flag and now its flipped.

CHANGELOG.md Outdated

### Enhancements

- Enha: Exceptions not handled by user code should be `unhandled: true` #1186 ([#1186](https://github.com/getsentry/sentry-dart/pull/1186))
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 🚫 The changelog entry seems to be part of an already released section ## 7.0.0.
    Consider moving the entry to the ## Unreleased section, please.

@denrase
Copy link
Collaborator Author

denrase commented Dec 19, 2022

@marandaneto Also removing them from unhandled but not crashing event mechanisms that we create in the SDK.

@denrase denrase requested a review from marandaneto December 19, 2022 12:43
@marandaneto
Copy link
Contributor

@marandaneto Also removing them from unhandled but not crashing event mechanisms that we create in the SDK.

Not sure I understand this comment, mind elaborating?

Another pending point if not done yet, we have to test the integration with the Native SDKs and the release health feature.

@denrase
Copy link
Collaborator Author

denrase commented Dec 20, 2022

@marandaneto Sorry, hope this is a better explanation.

Not sure I understand this comment, mind elaborating?

Ah sure, i was referring to the documentation of why we introduced the breadcrumb removal.

We then remove the breadcrumbs in all cases but if it is handled == false, this is a signal that the app would crash and android would lose the breadcrumbs by the time the app is restarted to read the envelope.

My assumption is that we still need to remove the breadcrumbs in those new instances where we have set the value to handled == false, as the app is not crashing on flutter.

I have added unit tests that cover this new behaviour.

// final hasOnlyHandledMechanism = mechanisms
// .every((e) => (e.handled ?? true));

// if (hasNoMechanism || hasOnlyHandledMechanism) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marandaneto Is this final or are you still in implementation process?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just testing and found out that this has to be fixed, hence the comments.

@denrase
Copy link
Collaborator Author

denrase commented Jan 10, 2023

@marandaneto Raised an issue on the cocoa side.

@marandaneto
Copy link
Contributor

@marandaneto Raised an issue on the cocoa side.

Thanks, let's pause this PR for a bit, I'm talking to the Apple folks about this issue, this is a blocker for us.

@marandaneto marandaneto marked this pull request as draft January 25, 2023 12:41
Base automatically changed from v7.0.0 to main March 15, 2023 12:41
@marandaneto
Copy link
Contributor

@denrase this is unblocked now, getsentry/sentry-cocoa#2593

@marandaneto marandaneto added this to the 8.0.0 milestone Jun 15, 2023
@marandaneto
Copy link
Contributor

Closing in favor of #1535

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.

Exceptions not handled by user code should be unhandled: true

5 participants