Log unhandled FiatProviderError errors #5326
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
swansontec
approved these changes
Oct 25, 2024
src/util/tracking.ts
Outdated
Comment on lines
163
to
168
| let hint: { event_id?: string; data?: { [key: string]: any } } | undefined | ||
| if (tag != null) { | ||
| hint = { event_id: tag } | ||
| } | ||
| if (metadata != null) { | ||
| hint = { data: metadata } |
There was a problem hiding this comment.
This doesn't let us have both a tag and a metadata at the same time. Maybe do something like this:
let hint: { event_id?: string; data?: { [key: string]: any } } = {}
if (tag != null) hint.event_id = tag
if (metadata != null) hint.data = metadata
captureException(err, tag != null || metadata != null ? hint : undefined)There was a problem hiding this comment.
This logic is replaced because it turns out it didn't do what we expected it to do for the events we send to Sentry. In some cases, it harmed us.
Comment on lines
111
to
115
| // Recurse over objects with added prefix keys: | ||
| if (typeof value === 'object' && value !== null) { | ||
| addMetadataToContext(context, value, allKeys) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Should we just JSON.stringify instead of recursing? Imagine you have something like this:
{
fromCoin: { tokenId: 'asf', pluginId: 'asdf' },
toCoin: { tokenId: 'dda', pluginId: 'asafadf' }
}
The fields in toCoin would stomp over the fields in fromCoin, and you would lose information. If we use JSON.stringify on the values, then you would get:
{
fromCoin: '{"tokenId":"asf","pluginId":"asdf"}',
toCoin: '{"tokenId":"dda","pluginId":"asafadf"}'
}
This would be more usable, I think.
There was a problem hiding this comment.
The reason why: JSON strings are big and when metadata fields are too big, they're filtered out by Sentry. So, I flattened the JSON objects into separate fields. Each field contains the parent key in it's name so there shoulnd't be possibility of key stomping
samholmes
force-pushed
the
sam/fiat-error-tracking
branch
from
bf133f2
to
e67c036
Compare
1. Isolation normalization into new utility to de-dup error handling 2. Use `normalizeError` util at the end (trackError) 3. Fully remove i18n from error objects; only present localized strings in the UI. Translating error messages is be a locale issue. Maintaining our errors in the default language is more consistent and consistency is better when it comes to error tracking. If "to translate" an error means something other than i18n, such as to translate the error object and any special properties into a useful message for reporting, then a new function should be introduced for that purpose (call it `encodeError` or something of that nature).
Should an array be thrown, we'll normalize into an AggregateError type of object, and we'll concatenate localized error messages for i18n.
samholmes
force-pushed
the
sam/fiat-error-tracking
branch
from
e67c036
to
e594c92
Compare
The hint data is incorrectly implemented and causing major issues: 1. `event_id` is a hex value and any other value cause the error to not be reported. 2. `data` in the hint is only used locally before sending the event to Sentry. Instead we will use the `scope` API to add the proper event tagging and metadata. We can leverage the `addMetadataToContext` function to consistently process metadata for a Sentry event.
The name was the message, and the message didn't exist. Now we'll create a useful message from the info parameter.
samholmes
force-pushed
the
sam/fiat-error-tracking
branch
from
e594c92
to
d6d94a3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
#5322 (review)
Requirements
If you have made any visual changes to the GUI. Make sure you have: