feat(ember): Extract ember-specific logic into custom browserTracingIntegration#20702
Conversation
size-limit report 📦
|
mydea
force-pushed
the
fn/ember-better-browserTracing
branch
from
705bcf0 to
bc9ce81
Compare
| afterAllSetup(client: BrowserClient) { | ||
| integration.afterAllSetup(client); | ||
|
|
||
| // Run this in the next tick to ensure the ember router etc. is properly initialized |
There was a problem hiding this comment.
without this, the pageload instrumentation does not work. it seems that router.location is not properly resolved/available yet at this time. probably some timing issue. this was not a problem before because we had an await import before this which lead to no timing issues in that case.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
mydea
force-pushed
the
fn/ember-better-browserTracing
branch
from
14cb4fa to
39a52a7
Compare
|
👋 @getsentry/team-javascript-sdks-framework — Please review this PR when you get a chance! |
make it work with legacy stuff... move stuff around and fix it WIP move stuff around... fix where stuff lives
mydea
force-pushed
the
fn/ember-better-browserTracing
branch
from
39a52a7 to
9fc40f2
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| } | ||
| if (!disableInitialLoadInstrumentation) { | ||
| _instrumentInitialLoad(); | ||
| } |
There was a problem hiding this comment.
Performance marks not cleared when instrumentation disabled
Low Severity
When disableInitialLoadInstrumentation is true, _instrumentInitialLoad() is never called. The old code always called this function regardless, and it would explicitly clearMarks for @sentry/ember:initial-load-start and @sentry/ember:initial-load-end before returning early. The new code skips the function entirely, so those performance marks persist in the browser's performance timeline instead of being cleaned up.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9fc40f2. Configure here.
There was a problem hiding this comment.
this is fine, this is not added in the first place here.
| disableInitialLoadInstrumentation?: boolean; | ||
| }; | ||
|
|
||
| let _initialized = false; |
There was a problem hiding this comment.
Bug: The module-level _initialized flag in browserTracingIntegration.ts is not reset between tests, causing instrumentGlobalsForPerformance to be skipped in subsequent test runs, leading to inconsistent behavior.
Severity: LOW
Suggested Fix
The _initialized flag should be reset between test runs. This can be achieved by exporting a reset function from the browserTracingIntegration.ts module and calling it in a beforeEach or afterEach hook within the test setup helpers, such as setupSentryTest.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/ember/addon/utils/browserTracingIntegration.ts#L23
Potential issue: In a test environment, the `_initialized` flag in
`browserTracingIntegration.ts` is a module-level variable that is set to `true` on its
first execution and is never reset. If an Ember application instance is recreated
between tests, the `browserTracingIntegration` function will be called again. However,
because `_initialized` remains `true`, the call to `instrumentGlobalsForPerformance` is
skipped on subsequent runs. This can lead to inconsistent global instrumentation across
tests, potentially causing flaky or incorrect test outcomes for performance monitoring
features.
| // We only want to run this once in tests! | ||
| if (macroCondition(isTesting())) { | ||
| if (_initialized) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| instrumentGlobalsForPerformance(globalsPerformanceConfig); | ||
| _initialized = true; | ||
| }); |
There was a problem hiding this comment.
Bug: Using setTimeout() for deferred instrumentation creates a race condition in Ember tests, as it's not tracked by Ember's settled() state, causing assertions to run prematurely.
Severity: MEDIUM
Suggested Fix
Replace setTimeout() with an Ember-aware scheduling function like Ember.run.later(). This ensures the deferred work is tracked by Ember's test infrastructure, preventing the race condition and allowing tests to wait for instrumentation to complete before running assertions. Alternatively, re-expose a promise that resolves after instrumentation is complete, which tests can explicitly await.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/ember/addon/utils/browserTracingIntegration.ts#L75-L84
Potential issue: The use of `setTimeout()` in `browserTracingIntegration.ts` to defer
performance instrumentation creates a race condition in Ember acceptance tests. Ember's
test helpers, such as `await visit()`, rely on a `settled()` state which waits for the
run loop and tracked timers to complete. However, `setTimeout()` schedules work outside
of the Ember run loop, so it is not tracked. Consequently, test assertions can execute
before the deferred instrumentation code runs. This can cause tests that check for
transaction spans created by this instrumentation to fail because the spans have not yet
been created when the assertions are made.
Also affects:
packages/ember/addon/instance-initializers/sentry-performance.ts:15~21
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2ab8cf4. Configure here.
| }); | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Missing integration or E2E tests for new feature
Low Severity
This is a feat PR that introduces a new ember-specific browserTracingIntegration with significant refactored logic (new integration wrapper, setTimeout deferral, module-level _initialized guard, split config handling). However, the PR does not include any new integration or E2E tests for this new functionality. Existing tests were only updated for import path changes. Adding at least one integration test verifying the new integration's behavior would help catch regressions.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 2ab8cf4. Configure here.
3 participants
Kind of extracted/inspired by #19229, this refactors the ember logic for performance monitoring to use the more general pattern we already have in our code, delegating this fully into an ember-specific
browserTracingIntegration.For now, this should not be breaking, as the auto-wiring etc. remains the same - it just uses this integration under the hood instead of manually calling stuff etc.
In a follow up, this should make it much easier to use this in a more standalone fashion in the v2 addon.
I've split the logic into separate files as this was all a bit convoluted together, hopefully this makes it a bit easier to follow what belongs where etc.
This also generally aligns the ember package more with our own broader way of handling things, which is also nice IMHO. If you add the integration, you get everything set up, and if not, not - easy! :D
Status quo
This is still being added automatically - this is what
instance-initializers/sentry-performance.tsdoes. that auto-runs and adds the browser tracing integration so it has access to the ember app instance, which we need to setup tracing stuff properly.Future (v11)
We'll no longer have the auto instance-initializer, but instead users will have to manually add this. but this can be simplified then in their app code to: