Uh oh!
There was an error while loading. Please reload this page.
feat(ember)!: Update to v2 addon format - #23252
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| const globalsPerformanceConfig = { | ||
| disableRunloopPerformance: options.disableRunloopPerformance ?? false, | ||
| minimumRunloopQueueDuration: options.minimumRunloopQueueDuration, | ||
| minimumRunloopQueueDuration: options.minimumRunloopQueueDuration ?? 0, |
There was a problem hiding this comment.
Idle timeout default dropped
Medium Severity
The Ember integration no longer sets idleTimeout to 5000. It now inherits the browser default of 1000ms. Quiet stretches during a transition (for example a slow model hook without child spans) can finish the navigation/pageload span early, truncating Ember route timing that previously waited up to 5 seconds.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1f20412. Configure here.
4b990e8 to
4dcd6deCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
size-limit report 📦
|
4dcd6de to
6884073Compare| "@babel/plugin-transform-runtime@7.18.10", "@babel/plugin-transform-runtime@^7.13.9": | ||
| "@babel/plugin-transform-runtime@7.18.10": |
There was a problem hiding this comment.
Critical severity vulnerability may affect your project—review required:
Line 2594 lists a dependency (@babel/plugin-transform-runtime) with a known Critical severity vulnerability.
ℹ️ Why this matters
Affected versions of @babel/traverse and babel-traverse are vulnerable to Incomplete List of Disallowed Inputs / Incorrect Comparison. Compiling untrusted code with Babel using plugins that invoke the internal path.evaluate() or path.evaluateTruthy() methods (for example @babel/plugin-transform-runtime, @babel/preset-env with useBuiltIns, or any polyfill‐provider plugin) allows a maliciously crafted AST to execute arbitrary code on the build machine during compilation.
To resolve this comment:
Check if you use Babel to compile untrusted JavaScript.
💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
You can view more details on this finding in the Semgrep AppSec Platform here.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
8a593fd to
01de976CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| "@embroider/addon-dev": "^8.3.0", | ||
| "@embroider/compat": "^4.1.17", | ||
| "@embroider/core": "^4.4.7", | ||
| "@embroider/macros": "^1.20.2", |
There was a problem hiding this comment.
This is e.g. still used in browserTracingIntegration, why was this moved to devDep?
| "lint:fix": "OXLINT_TSGOLINT_DANGEROUSLY_SUPPRESS_PROGRAM_DIAGNOSTICS=true oxlint . --fix --type-aware", | ||
| "lint": "OXLINT_TSGOLINT_DANGEROUSLY_SUPPRESS_PROGRAM_DIAGNOSTICS=true oxlint . --type-aware", |
There was a problem hiding this comment.
Why do we do OXLINT_TSGOLINT_DANGEROUSLY_SUPPRESS_PROGRAM_DIAGNOSTICS?
There was a problem hiding this comment.
good catch, I think we had this when this was written originally but since removed it 👍
| ### FastBoot / SSR | ||
| The performance instrumentation automatically detects FastBoot and disables client-side instrumentation during server rendering. No changes needed. |
There was a problem hiding this comment.
q: How is this done? The packages/ember/addon/instance-initializers/sentry-performance.ts was removed and it looks like this was responsible for this part?
There was a problem hiding this comment.
jup, true, I re-added the check to skip this in instrumentAppInstancePerformance
Uh oh!
There was an error while loading. Please reload this page.
| // TODO(v11): make this required | ||
| appInstance?: ApplicationInstance; |
There was a problem hiding this comment.
Should this change be added in the migration guide?
There was a problem hiding this comment.
this generally needs an entry in migration guide, let me add one!
There was a problem hiding this comment.
I see most of the test files are deleted: are those cases covered in E2E or somewhere else now?
There was a problem hiding this comment.
yes this test app was moved to e2e app instead of running inside of the package itself.
daf2510 to
cdc0622Compare| const { fromRoute, toRoute } = getTransitionInformation(transition, routerService); | ||
| // Store this here to be used, even if the active span has ended | ||
| getCurrentScope().setTransactionName(`route:${toRoute}`); |
There was a problem hiding this comment.
Avoidable getCurrentScope usage
Low Severity
This is more of an "is this necessary" check than a hard rule violation: getCurrentScope() is used here to set the transaction name. In multi-client setups that can touch the wrong scope or create an unintended current scope. A client/scope reference already available in this instrumentation path would be safer.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit cdc0622. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
cdc0622 to
b4f2a29CompareUh oh!
There was an error while loading. Please reload this page.
chargome
left a comment
There was a problem hiding this comment.
Just the dataCollection change otherwise LGTM
b4f2a29 to
5ed4189CompareUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5ed4189. Configure here.
| import { getBackburner } from './utils.ts'; | ||
| // Ember runloop queue names | ||
| type EmberRunQueues = 'actions' | 'afterRender' | 'destroy' | 'render' | 'routerTransitions' | 'sync'; |
There was a problem hiding this comment.
Dead initial-load instrumentation
Low Severity
contentFor initial-load scripts were removed, but _instrumentInitialLoad() still runs and looks for @sentry/ember:initial-load-* performance marks that are never written now. The related disableInitialLoadInstrumentation option is therefore effectively inert dead code left behind by the migration.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5ed4189. Configure here.
Squashed from PR #19229 (getsentry/sentry-javascript). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5ed4189 to
a6851acCompare| routerService.on('routeDidChange', transition => { | ||
| if (!transitionSpan || !activeRootSpan || transitionIsIntermediate(transition)) { | ||
| return; |
There was a problem hiding this comment.
Bug: When instrumentNavigation is false, the routeDidChange handler exits prematurely, preventing the pageload span from being closed and causing a span leak.
Severity: MEDIUM
Suggested Fix
Modify the early exit condition in the routeDidChange handler. The logic should be adjusted to ensure that the backburner callback, which finalizes activeRootSpan, is registered even when transitionSpan is not defined. This will correctly close the pageload span in configurations where navigation instrumentation is disabled.
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/src/utils/instrumentEmberAppInstanceForPerformance.ts#L144-L146
Potential issue: When `instrumentNavigation` is set to `false` while page load
instrumentation is enabled, a logic flaw causes a pageload span leak. On initial load, a
pageload span (`activeRootSpan`) is created. The `routeWillChange` handler does not end
it. Subsequently, the `routeDidChange` handler returns early because `transitionSpan` is
undefined (due to `instrumentNavigation: false`). This early exit prevents the
registration of a `backburner` callback, which is the sole mechanism responsible for
closing `activeRootSpan`. Consequently, the pageload span remains open indefinitely,
leading to a resource leak.
Uh oh!
There was an error while loading. Please reload this page.
| instrumentEmberAppInstanceForPerformance( | ||
| client, | ||
| appInstance, | ||
| appInstancePerformanceConfig, | ||
| startBrowserTracingPageLoadSpan, | ||
| startBrowserTracingNavigationSpan, | ||
| ); |
There was a problem hiding this comment.
Bug: The instrumentEmberAppInstanceForPerformance function is no longer guarded against re-initialization in tests, causing duplicate router event listeners to be registered in test suites.
Severity: MEDIUM
Suggested Fix
Move the instrumentEmberAppInstanceForPerformance() call inside the if (macroCondition(isTesting())) { if (_initialized) return; } guard. This ensures the function is only executed once during test runs, preventing the registration of duplicate event listeners.
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/src/utils/browserTracingIntegration.ts#L57-L63
Potential issue: The `instrumentEmberAppInstanceForPerformance` call was moved outside
of the `_initialized` guard that prevents re-initialization in test environments.
Consequently, if tests re-initialize the integration, router event listeners for
`routeWillChange` and `routeDidChange` will be registered multiple times. This causes
duplicate event handler executions for each route transition, leading to the creation of
duplicate spans and potential test failures.
Uh oh!
There was an error while loading. Please reload this page.
Rebased continuation of #19229 (original author @aklkv) onto current `develop`, with merge conflicts resolved and the failing tests fixed. Migrates `@sentry/ember` from the legacy v1 addon format to the [Ember v2 addon format](https://rfcs.emberjs.com/id/0507-embroider-addons), so the package works with both classic Ember builds and Embroider-optimized builds and no longer depends on `@embroider/macros` at runtime. See #19229 for the full description of the migration. ## Why a new branch The original PR was ~1900 commits behind `develop` and conflicting. Rather than resolve the same conflicts twice across its two commits, I squashed them into one and rebased against the final state once. The subtle part: the migration renamed `packages/ember/addon/` → `src/`. Git therefore saw develop's later behavioral changes to those files as edits to *deleted* files and did **not** surface them as conflicts. Several develop-side changes had to be ported into the new `src/` files by hand: - **Span ops** (#22669, #23086) — route hooks now emit `op: 'function'` with a `code.function.name` attribute; the runloop uses `ui.task`; the transition span uses `router`. `instrumentRoutePerformance.ts` still carried the old `ui.ember.route.*` ops and had to be updated. - **URL attributes** (#22095, #22415) — `url.path` / `url.full` / `url.template` on router spans, reconciled onto the PR's restructured `instrumentEmberAppInstanceForPerformance.ts`. ## Build/tooling reconciliation - Re-added the nested `typescript: ~5.8.0` devDependency pin. `develop` upgraded to TypeScript 7 (the native compiler, which drops `typescript/lib/tsc`), and glint's declaration build needs the classic JS compiler — the same stop-gap `develop` already applies to ember (see #19435). Without the pin the declaration build fails with `ERR_PACKAGE_PATH_NOT_EXPORTED`. - Bumped `@sentry/browser` / `@sentry/core` from the PR's stale `10.53.1` to `10.67.0` and added `@sentry/conventions` (now imported by the ported instrumentation). - Removed the PR's `import/no-unresolved` oxlint rule (doesn't exist in this repo's oxlint 1.75) and wrapped `URL_FULL` in `filterCollectedUrl()` for the `sdk/no-unfiltered-url-attributes` rule, which now applies since the code lives under `src/**`. ## Test fixes The originally-failing tests came down to three things: - **Span-op port** above — fixed the `captures correct spans for navigation` assertions. - **Missing `traceLifecycle: 'static'`** in the two new e2e apps (`ember-strict-resolver`, `ember-vite`). `develop` made span-streaming the default and disables it in the ember test apps (#22588); the new apps predated that, so their performance tests hung waiting for transaction events that never arrived under streaming. - **Stale assertions** in `ember-strict-resolver`'s `sentry-performance.test.ts`, updated from the old `ui.ember.*` op schema to the new `router` / `function` / `ui.task` ops. All four ember e2e apps pass (`ember-classic` 6/6, `ember-embroider` 6/6, `ember-strict-resolver` 10/10, `ember-vite` 5/5), along with the ember unit tests, lint, and build. Supersedes #19229. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebased continuation of #19229 (original author @aklkv) onto current
develop, with merge conflicts resolved and the failing tests fixed.Migrates
@sentry/emberfrom the legacy v1 addon format to the Ember v2 addon format, so the package works with both classic Ember builds and Embroider-optimized builds and no longer depends on@embroider/macrosat runtime. See #19229 for the full description of the migration.Why a new branch
The original PR was ~1900 commits behind
developand conflicting. Rather than resolve the same conflicts twice across its two commits, I squashed them into one and rebased against the final state once.The subtle part: the migration renamed
packages/ember/addon/→src/. Git therefore saw develop's later behavioral changes to those files as edits to deleted files and did not surface them as conflicts. Several develop-side changes had to be ported into the newsrc/files by hand:functionandui.taskspan ops for route hooks and runloop #22669, feat(angular,ember,sveltekit)!: Userouterspan op for frontend routers #23086) — route hooks now emitop: 'function'with acode.function.nameattribute; the runloop usesui.task; the transition span usesrouter.instrumentRoutePerformance.tsstill carried the oldui.ember.route.*ops and had to be updated.url.fullattribute to core fetch instrumentation #22415) —url.path/url.full/url.templateon router spans, reconciled onto the PR's restructuredinstrumentEmberAppInstanceForPerformance.ts.Build/tooling reconciliation
typescript: ~5.8.0devDependency pin.developupgraded to TypeScript 7 (the native compiler, which dropstypescript/lib/tsc), and glint's declaration build needs the classic JS compiler — the same stop-gapdevelopalready applies to ember (see chore(v11): Upgrade to TypeScript 7.0 #19435). Without the pin the declaration build fails withERR_PACKAGE_PATH_NOT_EXPORTED.@sentry/browser/@sentry/corefrom the PR's stale10.53.1to10.67.0and added@sentry/conventions(now imported by the ported instrumentation).import/no-unresolvedoxlint rule (doesn't exist in this repo's oxlint 1.75) and wrappedURL_FULLinfilterCollectedUrl()for thesdk/no-unfiltered-url-attributesrule, which now applies since the code lives undersrc/**.Test fixes
The originally-failing tests came down to three things:
captures correct spans for navigationassertions.traceLifecycle: 'static'in the two new e2e apps (ember-strict-resolver,ember-vite).developmade span-streaming the default and disables it in the ember test apps (test: Disable span streaming in remaining tests #22588); the new apps predated that, so their performance tests hung waiting for transaction events that never arrived under streaming.ember-strict-resolver'ssentry-performance.test.ts, updated from the oldui.ember.*op schema to the newrouter/function/ui.taskops.All four ember e2e apps pass (
ember-classic6/6,ember-embroider6/6,ember-strict-resolver10/10,ember-vite5/5), along with the ember unit tests, lint, and build.Supersedes #19229.