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
mydea
force-pushed
the
fn/browserTracingSpans
branch
from
448d33b to
43baead
Compare
|
If 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. |
|
I added some tests for this, makes sense overall! |
mydea
force-pushed
the
fn/browserTracingSpans
branch
from
1172281 to
1550d4c
Compare
…#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.