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

[docs] Don't crash page if an Ad crashes #27178

Merged
merged 2 commits into from Jul 8, 2021
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jul 8, 2021

Follow-up to #27173 and #27151 (review).

The page shouldn't crash if an Ad crashes. We get no ad impressions either way and a bad user impression when it does crash.

The only downside would be that we won't get any reports when it does crash. Therefore we send custom google-analytics events on crash. Ultimately a proper error monitoring service would do this (e.g. Sentry).

Preview: https://deploy-preview-27178--material-ui.netlify.app/components/alert/

Report (not sure if "custom reports" are user or org specific): https://analytics.google.com/analytics/web/#/my-reports/tV0Fi0LzQT6Pq3dtgC-dFQ/a106598593w159022482p160376982/
Report template: https://analytics.google.com/analytics/web/template?uid=iI67mnEsRg2zboHOKceoiw

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Jul 8, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jul 8, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against 5b90faa

@eps1lon eps1lon marked this pull request as ready for review July 8, 2021 08:45
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Google Analytics has a notion of tracking exceptions in https://developers.google.com/analytics/devguides/collection/gtagjs/exceptions. But its usage in the dashboard seems problematic, so I think that we can ignore it for now.

Note that in the Store, we use Google Tag Manager to handle the extra snippets. It adds a bit of overhead but makes it easier to add scripts on demand, like to configure Sentry. Not sure if it's relevant for the main website.

@eps1lon
Copy link
Member Author

eps1lon commented Jul 8, 2021

Google Analytics has a notion of tracking exceptions in developers.google.com/analytics/devguides/collection/gtagjs/exceptions. But its usage in the dashboard seems problematic, so I think that we can ignore it for now.

This looks ok for me with a custom report. Let me play with it a bit.

@eps1lon
Copy link
Member Author

eps1lon commented Jul 8, 2021

@oliviertassinari The link provided is about Google Tag Manager. That's not applicable to the docs, no?

@eps1lon
Copy link
Member Author

eps1lon commented Jul 8, 2021

Works locally with

function ThrowingAd() {
  throw new Error('test');
}

Screenshot from 2021-07-08 14-31-45

Good to merge @oliviertassinari ?

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

It looks simple enough. The downside is that we soften the strength of the feedback loop, delaying the feedback of 30 days (by the time we look at what's the latest ads amount) to catch major issues. We only look at the stats of G.A. when we work on something related to what we work on. But I'm fine, everything comes with a cost, and this cost seems to fly.

@eps1lon
Copy link
Member Author

eps1lon commented Jul 8, 2021

The downside is that we soften the strength of the feedback loop, delaying the feedback of 30 days (by the time we look at what's the latest ads amount) to catch major issues.

You don't know that yet. I've setup a weekly email digest for error events so the feedback loop may even be shorter.

@eps1lon eps1lon merged commit 8a4d032 into mui:next Jul 8, 2021
@eps1lon eps1lon deleted the docs/ads-graceful branch July 8, 2021 17:39
@eps1lon
Copy link
Member Author

eps1lon commented Jul 8, 2021

Screenshot from 2021-07-08 19-43-11

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 8, 2021

This is an interesting resolution, I didn't know this was possible. I hope we won't get false positives :)

@eps1lon
Copy link
Member Author

eps1lon commented Jul 8, 2021

This is an interesting resolution, I didn't this was possible. I hope we won't get false positives :)

Didn't enter your mail so you're safe. For now 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants