Metrics adjustments #15313
Metrics adjustments #15313
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [f6ae2f6]
Page Load Metrics (2429 ± 364 ms)
highlights: |
Builds ready [d7202b4]
Page Load Metrics (1990 ± 87 ms)
highlights: |
| }); | ||
|
|
||
| it('routes to select action when participateInMetaMetrics is not initialized', () => { | ||
| it('routes to the metametrics screen when participateInMetaMetrics is not initialized', () => { |
There was a problem hiding this comment.
For more coverage, we could add a test to check the INITIALIZE_CREATE_PASSWORD_ROUTE
| release, | ||
| beforeSend: (report) => rewriteReport(report), | ||
| beforeSend: (report) => { | ||
| if (getState) { |
There was a problem hiding this comment.
I just tried testing this, and there is still a call made to Sentry upon .init being called. This effectively prevents error reports from being sent, but it doesn't delay all communication. We will need to come up with a different strategy.
Typically I would suggest delaying the call to init. This would require handling captureException calls prior to init somehow (e.g. bring back those wrapper conditions to skip captureException pre-opt-in, or implement our own captureException wrapper function). But it wouldn't be that hard to do.
But for us that might not work, because I suspect that init has side-effects that do not work correctly under lockdown. It's possible they happen on import, not on init, I have not checked yet.. If they happen on init, we're in a tough spot. It means that if we call it later, it'll fail.
Assuming that is true, we might be able to effectively stop all communication by passing in a custom transport. The transport option can be used to handle proxies and things like that, so I would expect it to affect absolutely all communication, even the initial call.
Apparently init doesn't support the transport option anymore despite it being documented, but it can be set by creating our own Sentry client manually. (details here: getsentry/sentry-javascript#5422)
Another option would be to restart the process after opt-in, and somehow communicate to this bundle that the user opted in 🤔 Challenging since it would have to run before the controllers. This would certainly be more disruptive than the other option.
There was a problem hiding this comment.
I'm going to consider this non-blocking for this PR because what you have implemented so far is still an improvement, it's just not a complete solution yet.
There was a problem hiding this comment.
Okay, I will merge and then create a ticket to address the issues you have raised here.
ba1becb Don't send errors to sentry if users have not opted-in to participate in metametrics
31f0ffc Don't capture opt-out metrics
dfba519 Move the metrics-opt in screen to immediately after the welcome screen