Skip to content
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

Setup PublicEndpointFn option #2342

Merged
merged 2 commits into from Jul 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 12 additions & 1 deletion instrumentation/net/http/otelhttp/config.go
Expand Up @@ -38,6 +38,7 @@ type config struct {
Propagators propagation.TextMapPropagator
SpanStartOptions []trace.SpanStartOption
PublicEndpoint bool
PublicEndpointFn func(*http.Request) bool
ReadEvent bool
WriteEvent bool
Filters []Filter
Expand Down Expand Up @@ -108,7 +109,17 @@ func WithMeterProvider(provider metric.MeterProvider) Option {
func WithPublicEndpoint() Option {
return optionFunc(func(c *config) {
c.PublicEndpoint = true
c.SpanStartOptions = append(c.SpanStartOptions, trace.WithNewRoot())
})
}

// WithPublicEndpointFn runs with every request, and allows conditionnally
// configuring the Handler to link the span with an incoming span context. If
// this option is not provided or returns false, then the association is a
// child association instead of a link.
dashpole marked this conversation as resolved.
Show resolved Hide resolved
// Note: WithPublicEndpoint takes precedence over WithPublicEndpointFn.
func WithPublicEndpointFn(fn func(*http.Request) bool) Option {
return optionFunc(func(c *config) {
c.PublicEndpointFn = fn
})
}

Expand Down
23 changes: 14 additions & 9 deletions instrumentation/net/http/otelhttp/handler.go
Expand Up @@ -52,6 +52,7 @@ type Handler struct {
counters map[string]syncint64.Counter
valueRecorders map[string]syncfloat64.Histogram
publicEndpoint bool
publicEndpointFn func(*http.Request) bool
}

func defaultHandlerFormatter(operation string, _ *http.Request) string {
Expand Down Expand Up @@ -88,6 +89,7 @@ func (h *Handler) configure(c *config) {
h.filters = c.Filters
h.spanNameFormatter = c.SpanNameFormatter
h.publicEndpoint = c.PublicEndpoint
h.publicEndpointFn = c.PublicEndpointFn
}

func handleErr(err error) {
Expand Down Expand Up @@ -125,11 +127,21 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}
}

opts := append([]trace.SpanStartOption{
ctx := h.propagators.Extract(r.Context(), propagation.HeaderCarrier(r.Header))
opts := h.spanStartOptions
if h.publicEndpoint || (h.publicEndpointFn != nil && h.publicEndpointFn(r.WithContext(ctx))) {
opts = append(opts, trace.WithNewRoot())
// Linking incoming span context if any for public endpoint.
if s := trace.SpanContextFromContext(ctx); s.IsValid() && s.IsRemote() {
opts = append(opts, trace.WithLinks(trace.Link{SpanContext: s}))
}
}

opts = append([]trace.SpanStartOption{
trace.WithAttributes(semconv.NetAttributesFromHTTPRequest("tcp", r)...),
trace.WithAttributes(semconv.EndUserAttributesFromHTTPRequest(r)...),
trace.WithAttributes(semconv.HTTPServerAttributesFromHTTPRequest(h.operation, "", r)...),
}, h.spanStartOptions...) // start with the configured options
}, opts...) // start with the configured options

tracer := h.tracer

Expand All @@ -141,13 +153,6 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}
}

ctx := h.propagators.Extract(r.Context(), propagation.HeaderCarrier(r.Header))
if h.publicEndpoint {
// Linking incoming span context if any for public endpoint.
if s := trace.SpanContextFromContext(ctx); s.IsValid() && s.IsRemote() {
opts = append(opts, trace.WithLinks(trace.Link{SpanContext: s}))
}
}
ctx, span := tracer.Start(ctx, h.spanNameFormatter(h.operation, r), opts...)
defer span.End()

Expand Down
84 changes: 84 additions & 0 deletions instrumentation/net/http/otelhttp/test/handler_test.go
Expand Up @@ -190,6 +190,90 @@ func TestWithPublicEndpoint(t *testing.T) {
require.True(t, sc.Equal(done[0].Links()[0].SpanContext), "should link incoming span context")
}

func TestWithPublicEndpointFn(t *testing.T) {
remoteSpan := trace.SpanContextConfig{
TraceID: trace.TraceID{0x01},
SpanID: trace.SpanID{0x01},
TraceFlags: trace.FlagsSampled,
Remote: true,
}
prop := propagation.TraceContext{}

for _, tt := range []struct {
name string
fn func(*http.Request) bool
handlerAssert func(*testing.T, trace.SpanContext)
spansAssert func(*testing.T, trace.SpanContext, []sdktrace.ReadOnlySpan)
}{
{
name: "with the method returning true",
fn: func(r *http.Request) bool {
return true
},
handlerAssert: func(t *testing.T, sc trace.SpanContext) {
// Should be with new root trace.
assert.True(t, sc.IsValid())
assert.False(t, sc.IsRemote())
assert.NotEqual(t, remoteSpan.TraceID, sc.TraceID())
},
spansAssert: func(t *testing.T, sc trace.SpanContext, spans []sdktrace.ReadOnlySpan) {
require.Len(t, spans, 1)
require.Len(t, spans[0].Links(), 1, "should contain link")
require.True(t, sc.Equal(spans[0].Links()[0].SpanContext), "should link incoming span context")
},
},
{
name: "with the method returning false",
fn: func(r *http.Request) bool {
return false
},
handlerAssert: func(t *testing.T, sc trace.SpanContext) {
// Should have remote span as parent
assert.True(t, sc.IsValid())
assert.False(t, sc.IsRemote())
assert.Equal(t, remoteSpan.TraceID, sc.TraceID())
},
spansAssert: func(t *testing.T, _ trace.SpanContext, spans []sdktrace.ReadOnlySpan) {
require.Len(t, spans, 1)
require.Len(t, spans[0].Links(), 0, "should not contain link")
},
},
} {
t.Run(tt.name, func(t *testing.T) {
spanRecorder := tracetest.NewSpanRecorder()
provider := sdktrace.NewTracerProvider(
sdktrace.WithSpanProcessor(spanRecorder),
)

h := otelhttp.NewHandler(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
s := trace.SpanFromContext(r.Context())
tt.handlerAssert(t, s.SpanContext())
}), "test_handler",
otelhttp.WithPublicEndpointFn(tt.fn),
otelhttp.WithPropagators(prop),
otelhttp.WithTracerProvider(provider),
)

r, err := http.NewRequest(http.MethodGet, "http://localhost/", nil)
require.NoError(t, err)

sc := trace.NewSpanContext(remoteSpan)
ctx := trace.ContextWithSpanContext(context.Background(), sc)
prop.Inject(ctx, propagation.HeaderCarrier(r.Header))

rr := httptest.NewRecorder()
h.ServeHTTP(rr, r)
assert.Equal(t, 200, rr.Result().StatusCode)

// Recorded span should be linked with an incoming span context.
assert.NoError(t, spanRecorder.ForceFlush(ctx))
spans := spanRecorder.Ended()
tt.spansAssert(t, sc, spans)
})
}
}

hanyuancheung marked this conversation as resolved.
Show resolved Hide resolved
func TestSpanStatus(t *testing.T) {
testCases := []struct {
httpStatusCode int
Expand Down