-
Notifications
You must be signed in to change notification settings - Fork 5k
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
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
"env:e2e": "SEGMENT_HOST='https://api.segment.io' SEGMENT_WRITE_KEY='FAKE' SENTRY_DSN_DEV=https://[email protected]/0000000 yarn", | ||
"env:e2e": "SEGMENT_HOST='https://api.segment.io' SEGMENT_WRITE_KEY='FAKE' yarn", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>()._flags
then 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to use Boolean
here?
Quality Gate failedFailed conditions |
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.js
that 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