feat(ember)!: Use function and ui.task span ops for route hooks and runloop - #22669
Conversation
size-limit report 📦
|
msonnb
changed the title
function span op for route hooksfunction and ui.task span ops for route hooks and runloop
de8350a to
7ac0b5d
Compare
915c173 to
62b0ee0
Compare
7ac0b5d to
6c7646e
Compare
e55c2e0 to
7ae594a
Compare
7ae594a to
f9dc60e
Compare
Route hooks are functions, not their own op category. The hook name moves to `code.function.name` and `ember.route.hook`. Ref: JS-3105 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Match the `function` op and the new `code.function.name` / `ember.route.hook` attributes. Span finders discriminate on the hook name, since all hooks now share one op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Queue values are phase detail, and Ember allows custom queues. The queue moves to the `ember.runloop.queue` attribute. Ref: JS-3105 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Component renders use `ui.render`, component definition lookups use `function`, and the initial load uses `ui.mount`. Both component spans set `ui.component_name`. Ref: JS-3105 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f9dc60e to
d9b3e6c
Compare
nicohrubec
left a comment
nicohrubec
left a comment
There was a problem hiding this comment.
overall lgtm, just two questions about conventions
| attributes: { | ||
| [SENTRY_OP]: BROWSER_UI_TASK_SPAN_OP, | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.ember', | ||
| 'ember.runloop.queue': queue, |
There was a problem hiding this comment.
m: does ember.runloop.queue also need to be added to @sentry/conventions?
There was a problem hiding this comment.
does ember.runloop.queue also need to be added to @sentry/conventions
yes, all attributes sent from our SDKs should be in conventions
I'm not sure whether we want to add library-specific attributes to conventions
we should always use a more generic attribute if possible that isn't tied to a specific lib or framework. I'm not sure if there's a specific replacement for ember.runloop.queue. If there's none, we can add an ember-specific attribute. There's precedence for framework-specific attributes, for example, Remix.
There was a problem hiding this comment.
i'm not really familiar with ember but from a quick search i couldn't find any fitting generic attribute and from this guide it looks like the run queue is a pretty specific/unique concept, so yes we should probably add this ember-specific attribute to conventions then.
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.ember', | ||
| [SENTRY_OP]: GENERAL_FUNCTION_SPAN_OP, | ||
| [CODE_FUNCTION_NAME]: hookName, | ||
| 'ember.route.hook': hookName, |
There was a problem hiding this comment.
m: same for ember.route.hook
There was a problem hiding this comment.
do we need ember.route.hook at all? seems like this is already covered with the general code.function.name attribute 🤔
There was a problem hiding this comment.
true, same value twice seems redundant. I think we can remove it 👍
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 63b4326. Configure here.
| attributes: { | ||
| [SENTRY_OP]: op, | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.ember', | ||
| [UI_COMPONENT_NAME]: name, |
There was a problem hiding this comment.
New attribute lacks test coverage
Low Severity
Newly added ui.component_name on Ember component spans is never asserted. Acceptance checks only match op | description, so a regression that drops the attribute would still pass. Per the testing conventions rule, newly added payload data needs thorough assertions.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 63b4326. Configure here.
|
👋 @s1gr1d — Please review this PR when you get a chance! |
3 participants
Three Ember op cleanups:
instrumentRoutePerformanceusefunctioninstead ofui.ember.route.<hook>. The hook name is preserved incode.function.nameandember.route.hook.ui.taskinstead ofui.ember.runloop.<queue>. The queue is preserved inember.runloop.queue. Ember allows custom queues, so the suffix was unbounded.ui.render, component definition lookups usefunction, and the initial load usesui.mount. Both component spans setui.component_name.Since spans that used to be distinguished by op suffix now share one op, the test helpers and E2E span finders identify them by attribute instead.
Note
ui.mountis not in the conventions registry yet, so it is a literal with a TODO to swap in the constant once registered.Part of #22446