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

Add stringified payload to metametrics controller trackEvent error message #11166

Merged
merged 2 commits into from
May 24, 2021

Conversation

danjm
Copy link
Contributor

@danjm danjm commented May 24, 2021

This adds the stringified payload to the error message thrown when their is no event or category passed to the trackEvent method of the metametrics controller. This will make it easier to identify the source of the error.

@danjm danjm requested a review from a team as a code owner May 24, 2021 10:00
@danjm danjm requested a review from adonesky1 May 24, 2021 10:00
@danjm danjm force-pushed the useful-metametrics-error-message branch from 2b155b3 to 3c91e04 Compare May 24, 2021 10:34
@metamaskbot
Copy link
Collaborator

Builds ready [3c91e04]
Page Load Metrics (616 ± 27 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint44776494
domContentLoaded4146836145627
load4156846165627
domInteractive4146836145627

Gudahtt
Gudahtt previously approved these changes May 24, 2021
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

throw new Error('Must specify event and category.');
throw new Error(
`Must specify event and category. Passed payload was: ${JSON.stringify(
payload,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we track the user's MetaMetricsId in sentry context @Gudahtt? If so tracking the entire payload is problematic here for anonymity reasons. It's likely safe here to just include the event OR category and possibly an array of keys supplied in the payload versus the values.

Copy link
Member

Choose a reason for hiding this comment

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

I had the same thought, but it looks like the payload doesn't include the MetaMetricsId. That's in the options.

Copy link
Member

Choose a reason for hiding this comment

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

Oh but the properties and sensitiveProperties are a lot. Especially the sensitiveProperties - those explicitly should not be included for privacy reasons. Good point - I agree with your recommendation.

Copy link
Contributor

Choose a reason for hiding this comment

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

its also in the sentry appState context.
Screen Shot 2021-05-24 at 12 29 34 PM

@Gudahtt Gudahtt dismissed their stale review May 24, 2021 16:46

See Brad's review

@danjm
Copy link
Contributor Author

danjm commented May 24, 2021

@brad-decker @Gudahtt Addressed above comments in d6f355f

@metamaskbot
Copy link
Collaborator

Builds ready [d6f355f]
Page Load Metrics (833 ± 31 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint469875136
domContentLoaded6299358306431
load6309378336431
domInteractive6299348306431

@danjm danjm merged commit 1323a77 into develop May 24, 2021
@danjm danjm deleted the useful-metametrics-error-message branch May 24, 2021 19:27
ryanml pushed a commit that referenced this pull request May 24, 2021
…ssage (#11166)

* Add stringified payload to metametrics controller trackEvent error message

* Don't include full payload when throwing error on missing event or category in trackEvent
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.

4 participants