-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
otel: Improve OpenTelemetry trace instrumentation #7224
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
base: master
Are you sure you want to change the base?
Changes from all commits
e35ce3d
bb450ed
6c05741
5f9398c
8f40754
9f9d9c5
b6966f4
023999e
f704d0b
3d93ce4
f58b6fb
72ab14f
558e28d
ed087ec
ae1d4ef
ced6ba4
a0b1a8d
5ccad83
91778c5
1aac622
1c7d0e2
d3e97f8
9dfc038
e50e3ed
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 |
---|---|---|
|
@@ -33,4 +33,4 @@ | |
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ import ( | |
"sync" | ||
"time" | ||
|
||
"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" | ||
"go.uber.org/zap" | ||
"go.uber.org/zap/zapcore" | ||
"golang.org/x/net/http/httpguts" | ||
|
@@ -256,6 +257,7 @@ func (h *Handler) Provision(ctx caddy.Context) error { | |
if module, ok := h.Transport.(caddy.Module); ok && module.CaddyModule().ID.Name() == "fastcgi" && h.RequestBuffers == 0 { | ||
h.RequestBuffers = 4096 | ||
} | ||
h.Transport = otelhttp.NewTransport(h.Transport) | ||
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. I think if we reliably instrument all transports, we can stop mutating the inbound headers using |
||
} | ||
if h.LoadBalancing != nil && h.LoadBalancing.SelectionPolicyRaw != nil { | ||
mod, err := ctx.LoadModule(h.LoadBalancing, "SelectionPolicyRaw") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,8 @@ func init() { | |
type Tracing struct { | ||
// SpanName is a span name. It should follow the naming guidelines here: | ||
// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#span | ||
SpanName string `json:"span"` | ||
SpanName string `json:"span"` | ||
InjectServerTimingHeader bool `json:"server_timing,omitempty"` | ||
|
||
// otel implements opentelemetry related logic. | ||
otel openTelemetryWrapper | ||
|
@@ -46,7 +47,7 @@ func (ot *Tracing) Provision(ctx caddy.Context) error { | |
ot.logger = ctx.Logger() | ||
|
||
var err error | ||
ot.otel, err = newOpenTelemetryWrapper(ctx, ot.SpanName) | ||
ot.otel, err = newOpenTelemetryWrapper(ctx, ot.SpanName, ot.InjectServerTimingHeader) | ||
|
||
return err | ||
} | ||
|
@@ -68,6 +69,7 @@ func (ot *Tracing) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyh | |
// UnmarshalCaddyfile sets up the module from Caddyfile tokens. Syntax: | ||
// | ||
// tracing { | ||
// [server_timing] | ||
// [span <span_name>] | ||
// } | ||
func (ot *Tracing) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { | ||
|
@@ -94,12 +96,19 @@ func (ot *Tracing) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { | |
} | ||
|
||
for d.NextBlock(0) { | ||
if dst, ok := paramsMap[d.Val()]; ok { | ||
if err := setParameter(d, dst); err != nil { | ||
return err | ||
switch d.Val() { | ||
case "server_timing": | ||
if d.NextArg() { | ||
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. After reading up a bit on testing the feature myself, I think this condition isn't right. With this |
||
ot.InjectServerTimingHeader = true | ||
} | ||
default: | ||
if dst, ok := paramsMap[d.Val()]; ok { | ||
if err := setParameter(d, dst); err != nil { | ||
return err | ||
} | ||
} else { | ||
return d.ArgErr() | ||
} | ||
} else { | ||
return d.ArgErr() | ||
} | ||
} | ||
return nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import ( | |
|
||
"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" | ||
"go.opentelemetry.io/contrib/propagators/autoprop" | ||
"go.opentelemetry.io/otel" | ||
"go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc" | ||
"go.opentelemetry.io/otel/propagation" | ||
"go.opentelemetry.io/otel/sdk/resource" | ||
|
@@ -37,20 +38,19 @@ type openTelemetryWrapper struct { | |
|
||
handler http.Handler | ||
|
||
spanName string | ||
spanName string | ||
injectServerTimingHeader bool | ||
} | ||
|
||
// newOpenTelemetryWrapper is responsible for the openTelemetryWrapper initialization using provided configuration. | ||
func newOpenTelemetryWrapper( | ||
ctx context.Context, | ||
spanName string, | ||
) (openTelemetryWrapper, error) { | ||
func newOpenTelemetryWrapper(ctx context.Context, spanName string, injectServerTimingHeader bool) (openTelemetryWrapper, error) { | ||
if spanName == "" { | ||
spanName = defaultSpanName | ||
} | ||
|
||
ot := openTelemetryWrapper{ | ||
spanName: spanName, | ||
injectServerTimingHeader: injectServerTimingHeader, | ||
spanName: spanName, | ||
} | ||
|
||
version, _ := caddy.Version() | ||
|
@@ -64,7 +64,9 @@ func newOpenTelemetryWrapper( | |
return ot, fmt.Errorf("creating trace exporter error: %w", err) | ||
} | ||
|
||
ot.propagators = autoprop.NewTextMapPropagator() | ||
prop := autoprop.NewTextMapPropagator() | ||
otel.SetTextMapPropagator(prop) | ||
ot.propagators = prop | ||
|
||
tracerProvider := globalTracerProvider.getTracerProvider( | ||
sdktrace.WithBatcher(traceExporter), | ||
|
@@ -98,6 +100,11 @@ func (ot *openTelemetryWrapper) serveHTTP(w http.ResponseWriter, r *http.Request | |
extra.Add(zap.String("traceID", traceID)) | ||
extra.Add(zap.String("spanID", spanID)) | ||
} | ||
|
||
// Add the server-timing header so clients can make the connection | ||
if ot.injectServerTimingHeader { | ||
w.Header().Set("server-timing", fmt.Sprintf("traceparent;desc=\"00-%s-%s-%s\"", traceID, spanID, spanCtx.TraceFlags().String())) | ||
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. I think this should be |
||
} | ||
} | ||
next := ctx.Value(nextCallCtxKey).(*nextCall) | ||
next.err = next.next.ServeHTTP(w, r) | ||
|
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 actually broke the instrumentation in my example, because it now only works if
h.TransportRaw != nil
which isn't the case when using the default transport.