search

open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/
Apache License 2.0
4.99k stars 1.01k forks source link

trace.TraceContext stutters #448

Closed rghetia closed 3 years ago

rghetia commented 4 years ago

Currently golint warning on trace.TraceContext is disabled. Opened this issue to find a better name.

krnowak commented 4 years ago

Thanks for filing this issue for me.

To expand on it a bit: TraceContext used to be in propagators package, so there was no stutter, and it was clear that it was a propagator because of the package name. Now it has been moved to the trace package, so it stutters and the trace.TraceContext name does not make it clear that it is a propagator. The trace.B3 propagator is also affected - not clear that it is a propagator.

Also, we are going to have the same problem later when we split the correlation context propagation from trace.TraceContext into the correlation package - we will get something like correlation.CorrelationContext or something.

So to solve the issue, I'm proposing the following:

@rghetia also proposed naming trace.TraceContext as trace.ContextPropagator which, to me, sounds too generic, because trace.B3Propagator is also a context propagator.

rghetia commented 4 years ago
  • Since trace.TraceContextPropagator and correlation.CorrelationContextPropagator are awfully long and they stutter, we need to shorten/fix their names. Since both of them are implementations of some W3C spec, we could name them trace.W3CPropagator and correlation.W3CPropagator, respectively.

I like W3CPropagator.

iredelmeier commented 4 years ago

I'm personally wary of W3C as it feels somewhat un-obvious and is meant to be the standard. You wouldn't say W3CContentLength.

rghetia commented 4 years ago

@iredelmeier do you have any suggestion if not W3CPropagator?

MikeGoldsmith commented 4 years ago

I like the addition of adding Propagator as a suffix tot he types to help people understand the intent.

If W3C is the meant to be the default/standard, could we name it DefaultPropagator? I'm not a huge fan of this incase we wish to change the default to something else in the future.

krnowak commented 4 years ago

If W3C is the meant to be the default/standard, could we name it DefaultPropagator? I'm not a huge fan of this incase we wish to change the default to something else in the future.

DefaultPropagator is something else - we have this as a function for both span context and correlation context that returns a propagator.

MrAlias commented 4 years ago

we could name them trace.W3CPropagator and correlation.W3CPropagator, respectively

I'll throw my support behind this. I think the fully-qualified name (i.e. including the trace. and correlation. prefix) make the intent clear. To me it indicates the trace or correlation propagator defined by the W3C.

However, if including the standards body as part of the name is ultimately not desired, could we just use trace.Propagator and correlation.Propagator? The specification states these are the format to support:

Implementations MAY provide global Propagators for each supported Format.

If offered, the global Propagators should default to a composite Propagator containing W3C Trace Context and Correlation Context Propagators, in order to provide propagation even in the presence of no-op OpenTelemetry implementations.

Seems like just calling them *Propagator, while losing implementation details, is in line with the project.

jmacd commented 4 years ago

I think I would be OK with trace.Context.

MrAlias commented 3 years ago

Resolved by https://github.com/open-telemetry/opentelemetry-go/pull/1118