-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[confighttp]: add an option to add span formatter #11230
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11230 +/- ##
==========================================
- Coverage 91.48% 91.48% -0.01%
==========================================
Files 433 433
Lines 23617 23633 +16
==========================================
+ Hits 21607 21620 +13
- Misses 1642 1645 +3
Partials 368 368 ☔ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <andrzej.stencel@elastic.co>
5e2319c
to
805a7b5
Compare
805a7b5
to
dffbad7
Compare
@bogdandrutu @codeboten The CI looks good now. PTAL! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
otelhttp.WithSpanNameFormatter(func(operation string, r *http.Request) string { | |
if len(operation) > 0 { | |
return fmt.Sprintf("%s:%s", operation, r.URL.Path) | |
} | |
return r.URL.Path | |
}), |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmathieu since you are around, any idea of whether otelhttp
will be marked stable any time soon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sfc-gh-bdrutu @bogdandrutu can you take a look now? I've made it more generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Description
This PR adds a new server option to modify span formatter.
it also updates
otlpreceiver
to use the option.Link to tracking issue
Fixes #9382
Testing
Added.
component.ID
is not available in this section of code. SinceToServer
is intended to be general and can be used byextensions
,receivers
, andexporters
, modifying it to acceptinternal.Settings
would 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.