-
Notifications
You must be signed in to change notification settings - Fork 164
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
Proposal: clarify behavior when retrieving non-existent currently active span #216
Comments
What is the difference between noop and invalid span? |
There is this related (but not entirely applicable) point in the error handling guidance: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/error-handling.md#guidance
EDIT: I think this issue may not require an OTEP, if others agree, maybe move this issue to a normal spec issue. |
IMHO, their difference is an implementation detail. It's always a span which will not be sending any data once closed. Thank you for the error handling link. That would definitely point towards returning a noop span rather than nil. I think being able to know when those cases occur (with a warning for example) would be nice, as even though they shouldn't trigger exceptions, they should be catchable as well. |
regarding always return a span: How and a user differentiate between a non sampled trace (which is represented by Noop/NonRecording/... spans) and no trace active at all? This is relevant for example in |
What the Go SDK does is that SpanContext has an IsValid method, which returns false for noop spans. |
IsValid is part of the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#isvalid and it is unrelated to noop vs. not-noop. Instead it checks if the spancontext has a valid trace & span ID. I think that should satisfy your use case @Flarna, because a not-sampled span will have a valid span & trace ID. |
Well for propagators it's fine but I still don't see the advantage to create a Noop instance just to return something. Or to add more APIs on span like (IsSampled(), IsDummy(), IsApiOnly(), IsOnTrace(),...). What's wrong with It remembers me a bit on C++ |
The difference between nil and a noop span is that a noop span will accept calling all normal methods, while nil will throw exceptions on undefined methods. From the specifications mentioned above:
|
if you call a non existing method on a Noop it will also throw. So well, wrong usage results in undefined behavior - as one would expect. Maybe a bit off topic but related. Should we also return a dummy baggage if non is on context? and what should DummyBaggage.getEntry() return? at least in JS this returns Similar, what should |
I meant undefined methods for nil, not undefined for a proper span object. |
I think we should not issue a warning as it is perfectly fine that no trace is active. warning logs indicate that something is wrong so at most debug/info would should be used in my opinion. |
Sure, debug makes sense. I've updated the issue description. |
What I'm seeing all other SDKs do is return empty/noop baggage and metrics/tracer providers, which matches the specification statement. Regarding values, it seems to differ between SDKs. |
For languages that provide an implicitly propagated Context, the API should provide a way to retrieve the currently active span.
See https://github.com/open-telemetry/opentelemetry-specification/blob/2cfad37daf7e0d20851fd8a639a55375c3fc93dd/specification/trace/api.md#context-interaction
However, I am seeing a divergence in behaviours between SDKs which I believe would be nice to be coherent about.
If there is no current active span, most SDKs will return an invalid/noop span, while others will return undefined.
I believe this is a pretty big difference between those SDKs, as depending on the language being used, folks may get errors if they get a context which unexpectedly doesn't have any span.
Or they may be losing data if they get an invalid span and don't check for it.
My proposal is therefore the following:
The text was updated successfully, but these errors were encountered: