Uh oh!
There was an error while loading. Please reload this page.
feat(core): Make custom tracing methods return spans & set default op - #10633
Conversation
size-limit report 📦
|
| client.emit('startNavigationSpan', spanOptions); | ||
| const span = getActiveSpan(); | ||
| const op = span && spanToJSON(span).op; | ||
| return op === 'navigation' ? span : undefined; |
There was a problem hiding this comment.
Should we maybe have the emit hook return the span? Otherwise, we start racing again.
There was a problem hiding this comment.
so hooks so far have been designed with having no return value, always - which sometimes is a bit of a restriction but keeps the API very clean and slim. IMHO this should be very safe, we are in browser and there is only a single thread, there isn't really a way this could not work as far as I see, and type-wise we return Span | undefined anyhow so you have to write this defensively anyhow...?
There was a problem hiding this comment.
Also note that we can still change this later, as this is completely internal now - so if we figure this is not accurate enough, we can change how this works transparently :)
There was a problem hiding this comment.
I am not sure if this is clean and slim 🤔 Fine for now but to me this is very smelly.
There was a problem hiding this comment.
the hooks API itself is clean, in this concrete case it is not ideal of course - we can revisit this for sure!
This should make using these much easier. Also adjust the ember instrumentation to use these
448d33b to
43baeadCompareIf we already have tests, can we adjust them to check for the return value? If not, no worries, we'll check this implicitly with the routing instrumentation tests. |
mydea
commented
Feb 13, 2024
I added some tests for this, makes sense overall! |
1172281 to
1550d4cCompare…#10633) This should make using these much easier: * add a default `op` to the spans, so users don't need to specify them. * Return the created span (or undefined), ensuring users don't need to do all the checking for op etc. themselves. Also make some small adjustments to ember & angular instrumentation to leverage some of these changes.
…#10633) This should make using these much easier: * add a default `op` to the spans, so users don't need to specify them. * Return the created span (or undefined), ensuring users don't need to do all the checking for op etc. themselves. Also make some small adjustments to ember & angular instrumentation to leverage some of these changes.
…#10633) This should make using these much easier: * add a default `op` to the spans, so users don't need to specify them. * Return the created span (or undefined), ensuring users don't need to do all the checking for op etc. themselves. Also make some small adjustments to ember & angular instrumentation to leverage some of these changes.
This should make using these much easier:
opto the spans, so users don't need to specify them.Also make some small adjustments to ember & angular instrumentation to leverage some of these changes.