-
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?
Changes from 5 commits
cff8392
ce61bb5
c871eaf
06d3361
f2641b8
e4b5fb7
9171bb7
63d77c8
dffbad7
e6ab7d5
6018385
4fe1105
985a5d9
43d9d0d
f778f3a
e2d4dca
d1e3078
2deac0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
# Use this changelog template to create an entry for release notes. | ||
|
||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: enhancement | ||
|
||
# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) | ||
component: confighttp | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: Add an option to add prefix for span name for components | ||
|
||
# One or more tracking issues or pull requests related to the change | ||
issues: [9382] | ||
|
||
# (Optional) One or more lines of additional information to render under the primary note. | ||
# These lines will be padded with 2 spaces and then inserted directly into the document. | ||
# Use pipe (|) for multiline entries. | ||
subtext: | ||
|
||
# Optional: The change log or logs in which this entry should be included. | ||
# e.g. '[user]' or '[user, api]' | ||
# Include 'user' if the change is relevant to end users. | ||
# Include 'api' if there is a change to a library API. | ||
# Default: '[user]' | ||
change_logs: [] | ||
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,6 +13,7 @@ import ( | |||||||||||||
"net/http" | ||||||||||||||
"net/http/cookiejar" | ||||||||||||||
"net/url" | ||||||||||||||
"strings" | ||||||||||||||
"time" | ||||||||||||||
|
||||||||||||||
"github.com/rs/cors" | ||||||||||||||
|
@@ -377,6 +378,7 @@ func (hss *ServerConfig) ToListener(ctx context.Context) (net.Listener, error) { | |||||||||||||
type toServerOptions struct { | ||||||||||||||
errHandler func(w http.ResponseWriter, r *http.Request, errorMsg string, statusCode int) | ||||||||||||||
decoders map[string]func(body io.ReadCloser) (io.ReadCloser, error) | ||||||||||||||
spanPrefix string | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
// ToServerOption is an option to change the behavior of the HTTP server | ||||||||||||||
|
@@ -410,6 +412,14 @@ func WithDecoder(key string, dec func(body io.ReadCloser) (io.ReadCloser, error) | |||||||||||||
}) | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. We should make it a bit more generic and accept There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dmathieu since you are around, any idea of whether There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sfc-gh-bdrutu @bogdandrutu can you take a look? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A possible way around this is to move the option to a separate module. We are discussing this in #11769 |
||||||||||||||
|
||||||||||||||
// ToServer creates an http.Server from settings object. | ||||||||||||||
func (hss *ServerConfig) ToServer(_ context.Context, host component.Host, settings component.TelemetrySettings, handler http.Handler, opts ...ToServerOption) (*http.Server, error) { | ||||||||||||||
internal.WarnOnUnspecifiedHost(settings.Logger, hss.Endpoint) | ||||||||||||||
|
@@ -462,15 +472,14 @@ func (hss *ServerConfig) ToServer(_ context.Context, host component.Host, settin | |||||||||||||
otelOpts := []otelhttp.Option{ | ||||||||||||||
otelhttp.WithTracerProvider(settings.TracerProvider), | ||||||||||||||
otelhttp.WithPropagators(otel.GetTextMapPropagator()), | ||||||||||||||
otelhttp.WithSpanNameFormatter(func(_ string, r *http.Request) string { | ||||||||||||||
return r.URL.Path | ||||||||||||||
otelhttp.WithSpanNameFormatter(func(operation string, r *http.Request) string { | ||||||||||||||
return strings.TrimPrefix(fmt.Sprintf("%s:%s", operation, r.URL.Path), ":") | ||||||||||||||
VihasMakwana marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
}), | ||||||||||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This parameter is not a span prefix. It's the operation name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dmathieu I understand your perspective, but we use opentelemetry-collector/config/confighttp/confighttp.go Lines 474 to 479 in dffbad7
That's why I named it WithSpanPrefix .
Using For eg. Does that help clarify? |
||||||||||||||
|
||||||||||||||
// wrap the current handler in an interceptor that will add client.Info to the request's context | ||||||||||||||
handler = &clientInfoHandler{ | ||||||||||||||
|
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.
api