Add no-op implementations of Tracer API #98
Conversation
mayurkale22
requested review from
danielkhan,
iredelmeier,
justindsmith and
rochdev
as code owners
| /** | ||
| * No-op implementations of {@link Tracer}. | ||
| */ | ||
| export class NoopTracer implements Tracer { |
There was a problem hiding this comment.
I understand that NOOP_SPAN is useful to return for non-sampled cases, but what would be the use case for NoopTracer?
If it's intended to just get some code started, what would you think about calling this TracerBase or similar? Would this be the class that eventually takes a scope storage in the constructor?
There was a problem hiding this comment.
The main reason for NoopTracer, at least for us, is when users disable the tracer then the implementation is switched to the noop to avoid unnecessary overhead. The noop tracer then uses only noop classes for the entire API surface.
There was a problem hiding this comment.
+1 on @rochdev comment (#98 (comment))
And In OpenTracing there is a concept of Global tracer (which user can set using initGlobalTracer() function). AFAIK Global tracer (global.Tracer()) should return minimal Tracer - NoopTracer if the app chooses to NOT use the SDK like basic-tracer or auto-tracer or vendor specific.
packages/opentelemetry-core/src/context/propagation/NoopBinaryFormat.ts
Outdated
Show resolved
Hide resolved
| span: Span, | ||
| fn: T | ||
| ): ReturnType<T> { | ||
| throw new Error('Method not implemented.'); |
There was a problem hiding this comment.
Sure, added @todo for now (dependency on #100).
There was a problem hiding this comment.
I think we should discuss it on our next SIG the similar conversation here:
#99 (comment)
| import { NOOP_BINARY_FORMAT } from '../context/propagation/NoopBinaryFormat'; | ||
| import { NoopSpan } from './NoopSpan'; | ||
|
|
||
| export const NOOP_SPAN = new NoopSpan({ traceId: '', spanId: '' }); |
There was a problem hiding this comment.
Java uses specific values for invalid Span/Context:
https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/SpanContext.java#L34
https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/TraceId.java#L36
https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/SpanId.java#L36
I think it would make sense to match their behavior.
There was a problem hiding this comment.
Could we accomplish a similar thing by having say exprot const INVALID_TRACE_ID = '' and export const INVALID_SPAN_ID = '' in some authoritative place, maybe in the types package?
There was a problem hiding this comment.
Same thing is used in other PR : https://github.com/open-telemetry/opentelemetry-js/pull/102/files#diff-08ac79b280d2466a4fa3ffa58f1caed6R21
Will rebase, which ever is approved later.
Codecov Report
@@ Coverage Diff @@
## master #98 +/- ##
==========================================
- Coverage 98.67% 98.61% -0.07%
==========================================
Files 21 26 +5
Lines 1511 1732 +221
Branches 173 193 +20
==========================================
+ Hits 1491 1708 +217
- Misses 20 24 +4
|
| */ | ||
|
|
||
| export const INVALID_SPANID = '0'; | ||
| export const INVALID_TRACEID = '0'; |
There was a problem hiding this comment.
Should these be fully padded values? To my knowledge, there aren't many ways a single '0' can sneak into a span context.
There was a problem hiding this comment.
I think the idea was if we are not able to extract the SpanContext correctly, then assign INVALID_SPAN_CONTEXT with INVALID_TRACEID(0) and INVALID_SPANID(0).
Just noticed, in the HTTP plugin we are using _emptySpanContext, ideally we should have used INVALID_SPAN_CONTEXT there.
Closes #86
This is still work in progress as some dependencies are not yet merged (especiallyBinaryFormat(merged now)HttpTraceContext(merged now)