[confighttp]: add an option to add span formatter #11230
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11230 +/- ##
=======================================
Coverage 92.15% 92.15%
=======================================
Files 432 432
Lines 20253 20265 +12
=======================================
+ Hits 18664 18676 +12
Misses 1228 1228
Partials 361 361 ☔ View full report in Codecov by Sentry. |
|
@bogdandrutu @open-telemetry/collector-approvers I'd appreciate your review on this! |
There was a problem hiding this comment.
Other than the one nit, this looks sensible to me. Still definitely needs a look from @codeboten or @bogdandrutu who have more insight into this functionality.
Co-authored-by: Andrzej Stencel <[email protected]>
VihasMakwana
force-pushed
the
span-prefix
branch
from
5e2319c
to
805a7b5
Compare
VihasMakwana
force-pushed
the
span-prefix
branch
from
805a7b5
to
dffbad7
Compare
|
@bogdandrutu @codeboten The CI looks good now. PTAL! |
There was a problem hiding this comment.
LGTM. @bogdandrutu since this is related to a TODO you added, would you mind double checking that this looks good?
|
@bogdandrutu @open-telemetry/collector-maintainers can someone take a look here? |
config/confighttp/confighttp.go
Outdated
| return r.URL.Path | ||
| }), | ||
| otelhttp.WithMeterProvider(settings.LeveledMeterProvider(configtelemetry.LevelDetailed)), | ||
| } | ||
|
|
||
| // Enable OpenTelemetry observability plugin. | ||
| // TODO: Consider to use component ID string as prefix for all the operations. | ||
| handler = otelhttp.NewHandler(handler, "", otelOpts...) | ||
| handler = otelhttp.NewHandler(handler, serverOpts.spanPrefix, otelOpts...) |
There was a problem hiding this comment.
This parameter is not a span prefix. It's the operation name.
See https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp#NewHandler
How about naming the option WithOperationName instead of WithSpanPrefix?
There was a problem hiding this comment.
@dmathieu I understand your perspective, but we use spanPrefix specifically as a prefix for an operation in this context, using otelhttp.WithSpanNameFormatter
opentelemetry-collector/config/confighttp/confighttp.go
Lines 474 to 479 in dffbad7
That's why I named it
WithSpanPrefix.
Using WithOperationName might suggest that the option is meant for naming the operation itself, which isn’t accurate. Instead, it serves as a prefix.
For eg.
For example, if you specify WithSpanPrefix("foo") and make an HTTP request to http://host:port/path/bar, the tracer would be named foo:/path/bar.
Does that help clarify?
config/confighttp/confighttp.go
Outdated
| // WithSpanPrefix creates a span prefixed with the specified name. | ||
| // Ideally, this prefix should be the component's ID. | ||
| func WithSpanPrefix(spanPrefix string) ToServerOption { | ||
| return toServerOptionFunc(func(opts *toServerOptions) { | ||
| opts.spanPrefix = spanPrefix | ||
| }) | ||
| } |
There was a problem hiding this comment.
We should make it a bit more generic and accept otelhttp.Option or at least accept exactly same formatter? You may now want this, but later people may want other things, etc.
There was a problem hiding this comment.
The problem though, is that otelhttp is not stable, and we are looking to mark this stable soon. So I think we may not be able to accept this until otelhttp is marked stable.
There was a problem hiding this comment.
@dmathieu since you are around, any idea of whether otelhttp will be marked stable any time soon?
There was a problem hiding this comment.
@sfc-gh-bdrutu @bogdandrutu can you take a look now? I've made it more generic.
There was a problem hiding this comment.
any idea of whether otelhttp will be marked stable any time soon?
That is not on our radar at the moment. However, we are committed to keeping the API stable, or providing deprecation warnings for at least two versions.
VihasMakwana
changed the title
Description
This PR adds a new server option to modify span formatter.
it also updates
otlpreceiverto use the option.Link to tracking issue
Fixes #9382
Testing
Added.
component.IDis not available in this section of code. SinceToServeris intended to be general and can be used byextensions,receivers, andexporters, modifying it to acceptinternal.Settingswould introduce a breaking change, which I'm not comfortable with. Therefore, I believe it would be better to provide an option to change the prefix instead.