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

CrashLogging: New Error Logging API #274

Merged
merged 4 commits into from
Dec 18, 2023
Merged

Conversation

jleandroperez
Copy link
Contributor

Details:

We'll be using Sentry to track down Sync-Errors in DayOne. While testing the main integration PR, I've noticed it was next to impossible to filter by a given event's Additional Data fields.

You can find a Sample Event Here, with the Additional Data set (but no Tags)

Since Sentry recommends using Enriched Events, which can then be filtered with the keyword has:., in this PR we're adding an extra API that allows us to set a Tag/Value pair in the event's context.

cc @jkmassel (Thanks a lot in advance!!


  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

@jleandroperez jleandroperez added this to the 3.2.0 milestone Dec 16, 2023
@jleandroperez jleandroperez self-assigned this Dec 16, 2023

dataProvider.didLogErrorCallback?(event)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No easy way to unit test this. I've tinkered with the idea of adding didLogTaggedErrorCallback, but TBH the entire test felt pointless, since it would only be able to evaluate that the callback gets invoked 😆

/// - Parameters:
/// - error: The error object
/// - tagName: Tag name to be associated with the error
/// - tagValue: Tag's value to be associated with the error
Copy link
Contributor

Choose a reason for hiding this comment

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

The variables here (tagName & tagValue) don't match the function.

@jleandroperez
Copy link
Contributor Author

CI seems to be failing because of a couple new warnings, unrelated to this PR.
Those are being tracked in #273.

Merging this one back into Trunk. Thanks a lot Tony!!

@crazytonyli crazytonyli merged commit 3c0988d into trunk Dec 18, 2023
6 of 8 checks passed
@crazytonyli crazytonyli deleted the lantean/new-public-api branch December 18, 2023 21:39
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.

2 participants