Add Span API.#35
Conversation
g-easy
requested review from
jmacd,
maxgolov,
reyang,
rnburn and
tigrannajaryan
as code owners
|
|
||
| // Mark the end of the Span. Only the timing of the first end call for a given {@code Span} will | ||
| // be recorded, and implementations are free to ignore all further calls. | ||
| virtual void End() = 0; |
There was a problem hiding this comment.
Can we make these noexcept, especially something like End which is likely to be called from a destructor?
If any of these fail, I'd expect a tracer to turn them into noops.
There was a problem hiding this comment.
Done.
Considering "API can be used without exceptions", should every function in the API be marked noexcept explicitly?
|
|
||
| /** | ||
| * Noop implementation of Tracer | ||
| * No-op implementation of Tracer. This class should not be used directly. |
There was a problem hiding this comment.
Why shouldn't we use a no-op tracer directly?
There was a problem hiding this comment.
I assumed a binary using otel wouldn't explicitly create or set or use the NoopTracer. What's the use-case for it? (I removed the comment either way)
There was a problem hiding this comment.
I could imagine users wanting to use a different ownership model than the TracerProvider that creates one tracer per named component and ties lifetime to a global.
Envoy, for example, uses one tracer per thread
https://github.com/envoyproxy/envoy/blob/master/source/extensions/tracers/lightstep/lightstep_tracer_impl.cc#L156
There was a problem hiding this comment.
That makes sense.
What I was trying to do is steer users away from the NoopTracer specifically, and encouraging them to use the TracerProvider to get a tracer.
|
What else needs to be done here? |
| { | ||
| namespace trace | ||
| { | ||
| enum class SpanKind |
There was a problem hiding this comment.
Do we want kUnspecified? Would this be int or uint (or uint8)?
There was a problem hiding this comment.
IMO let's leave it as-is to match what Java does: https://github.com/open-telemetry/opentelemetry-java/blob/42bff3e9223181af800c76186744472c13499b0a/api/src/main/java/io/opentelemetry/trace/Span.java#L41
| // Adds an event to the Span, with a custom timestamp, and attributes. | ||
| // virtual void AddEvent(nostd::string_view name, core::SteadyTimestamp | ||
| // timestamp, nostd::span<std::pair<nostd::string_view name, AttributeValue | ||
| // value>> attributes) noexcept = 0; |
There was a problem hiding this comment.
This should be nostd::span<const std::pair<nostd::string_view, AttributeValue>> since it doesn't modify the std::pair's (and otherwise it won't work with const containers)
There was a problem hiding this comment.
Done.
Is std::pair allowed, ABI-wise, or do we need to add a nostd::pair class?
3 participants
No description provided.