-
Notifications
You must be signed in to change notification settings - Fork 792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Span api feels unintuitive and underdocumented #1792
Comments
We're aware the documentation is a little lacking. If you're willing to help us out there it would be greatly appreciated. This is something I will be focusing on in the next week or so because I want the docs to be more approachable when we launch API 1.0
For starters, the currently active span is the default. Not sure what version of the API you're using, but the
The spec explicitly states this should be the default behavior.
Yes this would definitely be possible according to the wording of the spec. I would probably modify it slightly to something like this (wondering why you had options in an array?). tracer.startActiveSpan('name', options, context.active(), (span) => {
doStuff();
span.end(); // the span may not always end at the end of the function, so it's important that the user has the option to end it whenever it makes sense.
}); |
I agree this is very confusing. I have been trying to create a child span for a long time now without any success. At least my attempted child spans to not show up as nested in jaeger. They do participate in the same trace which is nice but they don't get nested. Everywhere I search I find the example mentioned in the original post: span = tracer.startSpan('name', { parent: tracer.getCurrentSpan() })
tracer.withSpan(span, () => {
doStuff()
span.end()
}) However that does not work at all since:
This is my latest iteration of trying many permutations to replace the above non-working example: export function createChildSpan(operation: string): Span {
const tracer = opentelemetry.trace.getTracer("default");
const span = tracer.startSpan(operation);
opentelemetry.setSpan(opentelemetry.context.active(), span);
return span;
} If there is a way to create child spans that works I would really like to know how :-). Perferably creating child spans that participate in the automatic spans using EDIT: I have now come to the conclusion that there is no longer any way to directly set parent-child relationships between spans. Because you cannot say what the parent span is when creating a child span. Instead we must indirectly set the parent-child relationship using context. So when starting a new span you can specify a context and in that context there might be a parent span and if so that becomes the parent of the newly started span. Is that a correct conclusion? Previously when using opentracing I was sending along the parent span as parameter when making function calls and so I could create child spans in child functions by specifying the parent. I guess now if I want to be explicit and pass a parameter in my functions that parameter needs to be the context instead since that is what is holding the parent. So instead of keeping track of the parent and passing it along, I must now keep track of the parent (to be able to close it) and also the context that the parent was set in. Is that correct? Because I find no way of getting the context from the parent span. It seems only the |
Yes, that's correct. See OTel spec for more details https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#context-interaction Please note that OTel specification did the move from specifying a parent span to using the context everywhere. While this is for sure more complicated an not that intuitive it's more flexible. Anyhow, this ship has sailed as spec for tracing 1.0 is frozen meanwhile and implementations like OTel Js adapt and follow the spec which required breaking changes. |
Thanks for the clarification. I guess the reason this change was made is because having explicit parents does not work with automated tracing. You can pass down parent within your own code but not to the auto tracing. So I can see the benefit in having a single way to specify parent that also works together with auto-tracing. Btw, I found |
I was not that involved in the spec discussions regarding this. As far as I know the main reason was that ontext is more generic and it can be used to transfer more concerns and not just a parent Span/SpanContext.
yes, see also https://github.com/open-telemetry/opentelemetry-js#0140-to-0150 |
Not sure if this is the right place for this but https://opentelemetry.io/docs/js/instrumentation/ still references parent. I do feel like the
Any clarity would really help, on a greenfield project and want to set the right idioms early on. |
Thanks for bringing this up. For now you can see an updated doc in the API repo here.
Tracer is separate from context because it may propagate additional information like baggage, or in the future metrics/logs related information. There is a lot of context and history for this decision, too much for a single github comment really, but a great place to start here if you really want to understand the reasoning behind the decisions would be the really well written OTEP 66.
An interesting idea, and I'm glad it worked for your usecase, but you have completely lost the context created by |
YES! Coming from Go where it's |
I think we can probably consider this resolved, at least the major pain points, with the website docs (https://opentelemetry.io/docs/instrumentation/js/instrumentation/) getting an overhaul recently. Nearly all major concepts are now documented (with the easiest way suggested) and have a code sample you can copy/paste. I'm sure there's still some gaps though, so anything someone finds, please file an issue over at https://github.com/open-telemetry/opentelemetry.io and I'll see what I can do |
open-telemetry#1792) Better align with the other exports. Follow-up to open-telemetry#1772.
For the last couple of days I have tried (and mostly succeeded) to implement some basic Opentelemetry instrumentation but I also ran into some problems which might be related to issues with documentation and my understanding of how the api is supposed to work.
The documentation is rather ... minimal. Looking at https://opentelemetry.io/docs/js/instrumentation/ there only seems to be rather incomplete documentation about how the instrumentation is supposed to work. Topics like context (and its propagation),
withSpan()
, status and kind of spans are not mentioned at all. The moment somebody will start to work with nested spans the use ifstartSpan()
+span.end()
will not work anymore (as context is ignored) and there is no easy way for users to actually figure out what is going on as there is no warning about the lack of context.Some of the chosen defaults seem odd now that I have worked with Opentelemetry instrumentation a bit:
2.1 When creating a new span with
startSpan()
one has to explicitly specify the parent span. From my understanding you almost always want to want to be the currently active span to be the parent of the new span e.g.:tracer.startSpan(name, { parent: tracer.getCurrentSpan() } );
I currently cannot come up with an example where I would not want the specify a parent or use a different one. Shouldn't this be the default behavior then with the ability to override this in cases where it is necessary?
2.2
startSpan()
does not "activate" the new span and in order to do thatwithSpan()
needs to be called afterwards. Again given that you probably want to do this 99% of the time shouldn't this be the default?Combining all of the above I feel the
startSpan()
andwithSpan()
functions are too low-level and make the adding of instrumentation unnecessarily cumbersome. Would it be possible to add a utility function that combines the effects mentioned above to provide an API that is less flexible but covers the most likely use-case better?When looking at examples around the web I see the following usage as idiomatic:
Would it be possible to add a function that looks like this:
This function would:
doStuff()
with the proper expected contextspan.end()
after runningdoStuff()
To test this I created the following (naive and conceptional) implementation:
This is an instrumented function without
doSpan()
:and this is a version with the example
doSpan()
implementation:In a better implementation the callback function for
doSpan()
could receive the newly created span as an argument so thattracer.getCurrentSpan().setStatus({code: opentelemetry.StatusCode.OK});
can become
span.setStatus({code: opentelemetry.StatusCode.OK});
I feel this version is easier on the eye but also probably does what most people would expect by handling the parenting and context and always
end()
-ing the span (which the example in the documentation seems to forget). If you then need specifically different behavior regarding parenting or context you can fall back tostartSpan()
andwithSpan()
.The text was updated successfully, but these errors were encountered: