feat(ember)!: Update to v2 addon format#19229
Conversation
53dd617 to
822ed0a
Compare
nicohrubec
left a comment
There was a problem hiding this comment.
Hey, thanks for contributing. A few notes:
- This PR is way too large. Please split it up into smaller pieces so we can review it properly.
- Is there a way to get this done without introducing breaking changes? If no, then we need to wait with this until our next major.
Hi @nicohrubec, thanks for the feedback! I understand the concern about PR size. Unfortunately, the v1 → v2 addon migration is an atomic change — you can't incrementally migrate between formats. The build pipeline, entry point structure, and package.json layout must change together for the addon to function. That said, the core SDK functionality is unchanged — the same instrumentation, error handling, and performance tracking code. The changes fall into two categories:
The actual instrumentation logic in I'm happy to:
Regarding breaking changes — the v2 format requires explicit Let me know how you'd like to proceed! |
822ed0a to
4874c99
Compare
|
@aklkv thanks a lot for doing this!🙏 FWIW: While waiting for this, I realized I didn't use the performance part of the addon, and was able to just use the That also means that the 'breaking change' mentioned in the PR is a very welcome change to reduce the scope of what is delegated to the 'ember' part of the addon. Most is now just handled by the core apis, which it how it should be👍 (the link to Ember v2 addon format is broken in the pr description, btw) |
NullVoxPopuli
left a comment
There was a problem hiding this comment.
This is a good PR.
I understand the maintainer's perspective of wanting it split up tho.
The only way to do so would be rearranging the monorepo a bit to make a separate test app package.
This would mean the goal of that PR is to just delete the dummy app from the existing v1 addon.
That will still be a large pr, and i don't really see a way to get better than 2 large prs for this needed work.
Maintainers,
You mentioned breaking change timing - i suspect that is because all packages in this repo are released at once?
May i introduce you to https://github.com/release-plan/release-plan ?
It allows each package to be published independently with correct semver inference based on changes in git (and labels assessing impact on a pr) it helps a lot!
4874c99 to
34b116c
Compare
34b116c to
3f38914
Compare
3f38914 to
a52e388
Compare
a52e388 to
c99ff39
Compare
c99ff39 to
e2a465d
Compare
e2a465d to
1f697a9
Compare
1f697a9 to
5884677
Compare
|
I left my review and suggestions for this pull request in aklkv#1. Here's my message to @aklkv on Ember Discord:
|
7cbad07 to
03d2cd6
Compare
| import type ApplicationInstance from '@ember/application/instance'; | ||
| import { setupPerformance } from '@sentry/ember'; | ||
|
|
||
| export function initialize(appInstance: ApplicationInstance): void { |
There was a problem hiding this comment.
Hey, I am taking another look over this as we are getting closer to planning the next major, where this could go in.
Ideally, we'd align this more closely with how we handle sentry init in other places as well - which would be, to keep things centralized in Sentry.init(). Is there a way we can make this work reliably? I am thinking of something like this:
// app.ts
export default class App extends Application {
modulePrefix = config.modulePrefix;
podModulePrefix = config.podModulePrefix;
Resolver = Resolver;
}
Sentry.init({
// ...
integrations: [
Sentry.browserTracingIntegration({ emberApp: App })
]
});and somehow derive/wrap the necessary thing inside of this? That would be the ideal solution IMHO, or something along these lines. then we can also get rid of all the async import and special options for performance etc. and just pass this directly to the (ember-specific) browser tracing integration 🤔
There was a problem hiding this comment.
Or alternatively, if this cannot be made to work nicely, we could also do this in the initializer, which would be more "sentry native":
import * as Sentry from '@sentry/ember';
export function initialize(appInstance) {
Sentry.addIntegration(Sentry.browserTracingIntegration({
appInstance,
// other options here
});
}this would possibly loose tiny bits of timing info but should overall be likely OK...
Then, we need to export a custom browserTracingIntegration from the ember package here, similar to e.g. the vue browserTracingIntegration, where we can run the actual logic.
There was a problem hiding this comment.
I will look into PRing into this PR some changes for this!
| <meta name="description" content="" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
|
|
||
| <script>if(window.performance&&window.performance.mark){window.performance.mark('@sentry/ember:initial-load-start');}</script> |
There was a problem hiding this comment.
honestly if we are doing such a big refactor I'd just drop this stuff as built-in functionality - you can build this yourself if needed but likely for simplicity this can go away.
|
@mydea thank you for looking at this PR, I will circle back closer to the end of the week |
|
Kind-of extracted/refactored some of this out into a PR that is backwards compatible: #20702 once this is merged, I'll change this PR to use this instead, which should reduce the noise in this specific PR and keep things to a minimum. I'll also move the demo-app into a e2e test application, keeping to our general structure of things. then the ember package itself and the changes in this PR should be relatively reasonably small! |
cc80587 to
1b8bf6b
Compare
3dd2872 to
8870fb2
Compare
8870fb2 to
90b045f
Compare
|
👋 @getsentry/team-javascript-sdks-framework — Please review this PR when you get a chance! |
…Integration` (#20702) 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.ts` does. 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: ```js import * as Sentry from '@sentry/ember'; export function initialize(appInstance) { Sentry.addIntegration(Sentry.browserTracingIntegration({ appInstance })); } ```
90b045f to
854f981
Compare
5aff5c3 to
9d80849
Compare
|
👋 @getsentry/team-javascript-sdks-framework — Please review this PR when you get a chance! |
|
Opened a PR here towards this branch with some adjustments: aklkv#2 |
| fromRoute, | ||
| toRoute, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Router mount flag never set
High Severity
The refactor checks routerService._hasMountedSentryPerformanceRouting to avoid double router instrumentation but never sets it to true after registering listeners (the v1 code did). Each browserTracingIntegration afterAllSetup run attaches duplicate routeWillChange/routeDidChange handlers, causing duplicated navigation/pageload transactions.
Reviewed by Cursor Bugbot for commit 44712d2. Configure here.
| appInstance, | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
FastBoot guard removed
Medium Severity
The built-in instance initializer used to skip performance setup when service:fastboot isFastBoot is true. instrumentAppInstancePerformance has no equivalent guard, so SSR/FastBoot app instances can still register browser tracing and router listeners on the server despite UPGRADE.md claiming automatic FastBoot detection.
Reviewed by Cursor Bugbot for commit 44712d2. Configure here.
| _initialized = true; | ||
| }); | ||
| instrumentGlobalsForPerformance(globalsPerformanceConfig); | ||
| _initialized = true; |
There was a problem hiding this comment.
Missing macros runtime dependency
High Severity
browserTracingIntegration still imports @embroider/macros (isTesting, macroCondition), but v2 package.json no longer lists that package under dependencies and publish Babel config does not macro-strip it. Apps without @embroider/macros can fail at module load after upgrading.
Reviewed by Cursor Bugbot for commit 44712d2. Configure here.
| if (routerService._hasMountedSentryPerformanceRouting) { | ||
| // Routing listens to route changes on the main router, and should not be initialized multiple times per page. | ||
| return; |
There was a problem hiding this comment.
Bug: The guard _hasMountedSentryPerformanceRouting in instrumentEmberAppInstanceForPerformance is never set, causing duplicate event listeners to be registered on multiple calls, especially in tests.
Severity: HIGH
Suggested Fix
After the guard check at line 36, add the statement routerService._hasMountedSentryPerformanceRouting = true;. This will ensure that the instrumentation logic and event listener registration only run once per routerService instance, as intended by the existing guard mechanism.
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#L36-L38
Potential issue: The function `instrumentEmberAppInstanceForPerformance` includes a
guard `if (routerService._hasMountedSentryPerformanceRouting)` to prevent
re-initialization. However, the flag `_hasMountedSentryPerformanceRouting` is never set
to `true` after the check. Consequently, every call to this function, such as during
Ember's instance-initializers in test environments, will re-register `'routeWillChange'`
and `'routeDidChange'` event listeners. This leads to the creation of duplicate spans
for each route change and corrupted transaction data being sent to Sentry, along with
performance degradation from the redundant instrumentation.
- Convert from v1 addon to v2 addon using rollup + @embroider/addon-dev - Export ember-specific browserTracingIntegration (following Vue pattern) - Drop initial load instrumentation (constants, scripts, _instrumentInitialLoad) - Add UPGRADE.md migration guide - Create ember-vite e2e test app (Vite + @embroider/vite) - Update ember-classic and ember-embroider e2e apps to new API Co-Authored-By: Claude <[email protected]>
* improvements for ember-v2 * remove addon files?? * remove config fix version * fix test app * fixes and stuff * moar fixes * small ref * rename to strict resolver * add instrumentAppInstancePerformance * fix ember-vite app * fix type
44712d2 to
52d4ffa
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 6 total unresolved issues (including 4 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 52d4ffa. Configure here.
| startBrowserTracingPageLoadSpan: typeof startBrowserTracingPageLoadSpanType, | ||
| startBrowserTracingNavigationSpan: typeof startBrowserTracingNavigationSpanType, | ||
| ): void { | ||
| const { disableRunloopPerformance, instrumentPageLoad, instrumentNavigation } = config; |
There was a problem hiding this comment.
Wrong name on loading routes
Medium Severity
getCurrentScope().setTransactionName runs before transitionIsIntermediate returns, so intermediate loading/error transitions can overwrite the active transaction name (e.g. route:*.loading) used for errors captured during that transition.
Reviewed by Cursor Bugbot for commit 52d4ffa. Configure here.
|
|
||
| ### 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.
FastBoot guard removed
High Severity
The v1 instance initializer skipped performance setup when service:fastboot reported FastBoot; that guard was removed with no replacement in instrumentAppInstancePerformance, yet the upgrade guide still claims FastBoot is detected automatically.
Reviewed by Cursor Bugbot for commit 52d4ffa. Configure here.
| export function instrumentAppInstancePerformance( | ||
| appInstance: ApplicationInstance, | ||
| options?: Partial<Parameters<typeof browserTracingIntegration>[0]>, | ||
| ): void { | ||
| addIntegration( | ||
| browserTracingIntegration({ | ||
| ...options, | ||
| appInstance, | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: The guard _hasMountedSentryPerformanceRouting is checked but never set, allowing duplicate event listeners if instrumentEmberAppInstanceForPerformance is called multiple times.
Severity: MEDIUM
Suggested Fix
Set the _hasMountedSentryPerformanceRouting property on the router service to true within the instrumentEmberAppInstanceForPerformance function after the initial setup is complete. This will ensure the guard correctly prevents the function's logic from running more than once.
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#L81-L91
Potential issue: The function `instrumentEmberAppInstanceForPerformance` includes a
check on the `_hasMountedSentryPerformanceRouting` property to prevent duplicate
initialization and the registration of multiple event listeners. However, this property
is never set to `true` anywhere in the codebase. As a result, this guard is ineffective.
If `instrumentEmberAppInstanceForPerformance` is invoked more than once through any code
path, it will lead to the registration of duplicate event listeners, potentially causing
performance issues or incorrect behavior.
|
There are a bunch of bot review comments, most of them seem reasonable/accurate to me to work through! |


Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Summary
Migrates
@sentry/emberfrom the legacy v1 addon format to the Ember v2 addon format. This modernizes the package to work with both classic Ember builds and Embroider-optimized builds, and removes the runtime dependency on@embroider/macros.Changes
ember-clibuild pipeline withrollup(transpilation →dist/) +tsc(declarations →declarations/)addon-main.cjsusing@embroider/addon-shimas the addon entry pointember-addon.version: 2, updatedexportsmap, addedember-addon.app-jsmapping@embroider/macrosgetOwnConfig()with explicitSentry.init()calls and a newsetupPerformance()exportAPI changes:
init()is now called directly inapp.tsor an initializer (no moreENV['@sentry/ember']config)setupPerformance()must be called from an instance-initializer to opt into performance instrumentation<script>tags inindex.htmlUPGRADE.mdwith detailed migration guideTest & CI updates:
ember-classic,ember-embroider) to use v2 patterns:ENV['@sentry/ember']toSentry.init()inapp.ts<script>tags toindex.html.github/workflows/build.ymlCACHED_BUILD_PATHSfrompackages/ember/*.d.tstopackages/ember/dist+packages/ember/declarationsTypeScript fixes:
"types": ["ember-source/types"]totsconfig.publish.jsonfor Ember module resolution during declaration generation@ember/routing/-private/transition)instrumentFunctiongeneric to avoid tuple mismatch errorsBreaking Changes
ENV['@sentry/ember']config inconfig/environment.jsis no longer supported — useSentry.init()directlysetupPerformance()from an instance-initializer<script>tags inapp/index.htmlCloses JS-217