-
Notifications
You must be signed in to change notification settings - Fork 49
feat: add tracer.startActiveSpan() helper #14
Conversation
Add withSpan helper to execute a function on a context holding a given span.
Codecov Report
@@ Coverage Diff @@
## main #14 +/- ##
==========================================
+ Coverage 94.65% 94.71% +0.06%
==========================================
Files 39 39
Lines 505 511 +6
Branches 80 80
==========================================
+ Hits 478 484 +6
Misses 27 27
Continue to review full report at Codecov.
|
| */ | ||
| export function withSpan< | ||
| A extends unknown[], | ||
| F extends (...args: A) => ReturnType<F> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if F was also passed the span instance so you could write code like
withSpan(tracer.startSpan('name'), async (span: Span) => {
// ...
});and avoid having to assign the span to a variable first like:
let span = tracer.startSpan('foo');
withSpan(span, () => {...});
span = tracer.startSpan('bar');
withSpan(span, () => {...});There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main idea of putting a span into a context and set this context active is to avoid the need to pass spans around manually. Instead you fetch it via getSpan(context.active()).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, I don't mean so it's available to pass around to other functions. I just mean as a convenience in the "with" scope to avoid boilerplate like getSpan(context.active()).setAttribute('foo', 'bar') everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recommended solution for that would be
withSpan(tracer.startSpan('name', { attributes }), async () => {
// ...
});There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't a closure scope exactly providing what you want?
const span = tracer.startSpan('name');
withSpan(span, async () => {
// ... span is accessible here
});The current with() proposal allows to use any existing function without modification as you can pass thisArg and arguments as needed. Having a requirement to accept a span as first argument would be quite limiting or requires everyone to implement wrappers/adaptors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dyladan right, but often you want to set attributes and span status, events, etc. conditionally and then always finally span.end().
I'm not claiming it's not possible to do as is, but this PR is intended to be a convenience API so why not make it even more convenient? It also avoids having to name a bunch of variables and have them all in scope when dealing with concurrent operations (which could be pretty error prone):
const span1 = tracer.startSpan('part1');
const span2 = tracer.startSpan('part2');
await Promise.all(
withSpan(span1, async () => {
// ...
span1.end();
}),
withSpan(span2, async () => {
// ...
span2.end();
})
);vs
await Promise.all(
withSpan(tracer.startSpan('part1'), async (span: Span) => {
// ...
span.end();
}),
withSpan(tracer.startSpan('part2'), async (span: Span) => {
// ...
span.end();
})
);|
Based on the spec in
I think we should have |
|
closing and will open a new since suggested approach differs from original work and I cannot figure out |
fixes: open-telemetry/opentelemetry-js#1923
Continuing on @Flarna's work from #7
Though it seems like open-telemetry/opentelemetry-js#1923 is where namespaced vs non-namespaced decision will be made. I will post there and ask for a tl;dr as to which it should be.