perf: add Sentry tracing to CircleCI runs#26588
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. |
d23477f to
cb0212d
Compare
| "webpack:clearcache": "rimraf node_modules/.cache/webpack", | ||
| "postinstall": "yarn webpack:clearcache", | ||
| "env:e2e": "SEGMENT_HOST='https://api.segment.io' SEGMENT_WRITE_KEY='FAKE' SENTRY_DSN_DEV=https://fake@sentry.io/0000000 yarn", | ||
| "env:e2e": "SEGMENT_HOST='https://api.segment.io' SEGMENT_WRITE_KEY='FAKE' yarn", |
There was a problem hiding this comment.
@matthewwalsh0 I need it to do real Sentry reporting, is this an okay change to make? I defined SENTRY_DSN_DEV in the CircleCI env vars. Is there a better way to accomplish this?
There was a problem hiding this comment.
Would it be safer to set this inside .circleci/config.yml on the relevant jobs so devs can still run the tests locally without any Sentry overhead?
There was a problem hiding this comment.
I wrote that initial message 2 weeks ago and a few things have changed, but I think we have to decide -- do we want to respect what the developer set in their .metamaskrc for SENTRY_DSN_DEV, or do we want to override it for E2E tests? If they do not set SENTRY_DSN_DEV at all, it will go down the log('Skipped initialization'); path.
There was a problem hiding this comment.
Good point.
Maybe the safest compromise is update getSentryTarget to use the fake DSN if no SENTRY_DSN_DEV is defined and process.env.IN_TEST is set?
Meaning all the tests by default still use the fake DSN as before, but now the developer can also use their own as desired by these changes?
9eb4e3c to
5d93032
Compare
c9418ae to
969f352
Compare
Builds ready [969f352]
Page Load Metrics (1794 ± 83 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
d69b0c3 to
4718657
Compare
Builds ready [4718657]
Page Load Metrics (1769 ± 104 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [1f0f5db]
Page Load Metrics (1763 ± 124 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #26588 +/- ##
===========================================
- Coverage 70.15% 70.11% -0.04%
===========================================
Files 1425 1426 +1
Lines 49653 49683 +30
Branches 13891 13901 +10
===========================================
+ Hits 34833 34834 +1
- Misses 14820 14849 +29 ☔ View full report in Codecov by Sentry. |
…es, changed `circleci.enabled` to `true` or unset
f28395a to
b531b6a
Compare
b531b6a to
bd44443
Compare
Builds ready [bd44443]
Page Load Metrics (1844 ± 74 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
davidmurdoch
left a comment
There was a problem hiding this comment.
This look good. I've Approved, but left a question and one more nit/suggestion.
We should think and talk about following this PR up with a way to ensure that all this additional runtime logic gets compiled out when built for production.
| doNotForceSentryForThisTest?: boolean; | ||
| }; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/consistent-type-definitions -- you can't extend a type, we want this to be an interface |
There was a problem hiding this comment.
nit: I think you can do it with something like this placed in a browser.d.ts file in our /types directory.
// in app/scripts/lib/manifestFlags.ts
// use
browser.runtime.getManifest<ManifestFlags>()._flags
or
browser.runtime.getManifest<ManifestFlags | undefined>()._flagsthen in /types/browser.d.ts:
import 'webextension-polyfill';
declare module 'webextension-polyfill' {
// Extend the existing WebExtensionManifest to include _flags
namespace Manifest {
interface WebExtensionManifestWithFlags<Flags>
extends WebExtensionManifest {
_flags: Flags;
}
}
namespace Runtime {
interface Static {
// extend `getManifest` to allow for expecting a flags property
getManifest<Flags = never>(): [Flags] extends [never]
? Manifest.WebExtensionManifest
: Manifest.WebExtensionManifestWithFlags<Flags>;
}
}
}You wouldn't have to make it generic, you could also hard code the types in the /types/browser.d.ts, or just make it return something like Record<string, unknown> then cast _flags like you already are.
If you don't want to include | undefined in the generic you can set the _flags: Flags type in browser.d.ts to _flags?: Flags instead.
Again, just a nit.
app/scripts/lib/setupSentry.js
Outdated
| const { circleci } = getManifestFlags(); | ||
|
|
||
| if (circleci?.enabled) { | ||
| Sentry.setTag('circleci.enabled', Boolean(circleci.enabled)); |
There was a problem hiding this comment.
Why do you need to use Boolean here?
|
Builds ready [c48a4c8]
Page Load Metrics (1784 ± 94 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
This will allow CircleCI runs to report information to Sentry. It was very difficult to achieve, because there were specific E2E tests in
test/e2e/tests/metrics/errors.spec.jsthat could never pass with this code added.Before this PR, there was no way to pass runtime flags to the extension during tests. (Well okay, more precisely, you could change certain things through Fixtures, and you could change certain things through Ganache parameters, but both of these didn't load until late in the app load. Sentry is one of the first things to load, and it needed a way to get runtime flags very early in the app load.)
The way this works is a bit of a hack, but it's the only solution we could come up with that would load early enough:
helpers.withFixtures()function callssetManifestFlags(), which reads in the manifest file and parses itgetManifestFlags()to get the custom flagsRelated issues
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist