-
Notifications
You must be signed in to change notification settings - Fork 4
fix: use runtime.HTTPPattern for route extraction in spans #55
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
Changes from all commits
6755b18
0b64e49
91ad35b
f705ea1
03b739d
96f996b
567810b
ab78d2d
c810545
d0cc95c
e4c4757
be90f26
3e48e72
9e698f8
2edd9c7
13df802
f5b473c
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 |
|---|---|---|
|
|
@@ -225,7 +225,9 @@ func httpSpanAttributes(r *http.Request) []attribute.KeyValue { | |
| attrs := []attribute.KeyValue{ | ||
| semconv.HTTPRequestMethodKey.String(r.Method), | ||
| semconv.URLPath(r.URL.Path), | ||
| semconv.ServerAddress(host), | ||
| } | ||
| if host != "" { | ||
| attrs = append(attrs, semconv.ServerAddress(host)) | ||
| } | ||
| if port != "" { | ||
| if p, err := strconv.Atoi(port); err == nil { | ||
|
|
@@ -235,9 +237,17 @@ func httpSpanAttributes(r *http.Request) []attribute.KeyValue { | |
| if r.URL.RawQuery != "" { | ||
| attrs = append(attrs, semconv.URLQuery(r.URL.RawQuery)) | ||
| } | ||
| if r.URL.Scheme != "" { | ||
| attrs = append(attrs, semconv.URLScheme(r.URL.Scheme)) | ||
| scheme := r.URL.Scheme | ||
| if scheme == "" { | ||
| if proto := r.Header.Get("X-Forwarded-Proto"); proto != "" { | ||
| scheme = proto | ||
| } else if r.TLS != nil { | ||
| scheme = "https" | ||
| } else { | ||
| scheme = "http" | ||
| } | ||
| } | ||
| attrs = append(attrs, semconv.URLScheme(scheme)) | ||
| return attrs | ||
| } | ||
|
|
||
|
|
@@ -256,7 +266,18 @@ func tracingWrapper(h http.Handler) http.Handler { | |
| ) | ||
| rec := &statusRecorder{ResponseWriter: w, status: http.StatusOK} | ||
| w = rec | ||
| defer endSpan(serverSpan, rec) | ||
| defer func() { | ||
| if recovered := recover(); recovered != nil { | ||
| if !rec.wroteHeader { | ||
| rec.status = http.StatusInternalServerError | ||
| } | ||
| serverSpan.RecordError(fmt.Errorf("panic: %v", recovered)) | ||
|
ankurs marked this conversation as resolved.
|
||
| serverSpan.SetStatus(codes.Error, "panic") | ||
| endSpan(serverSpan, rec) | ||
| panic(recovered) | ||
| } | ||
| endSpan(serverSpan, rec) | ||
|
Comment on lines
+269
to
+279
|
||
| }() | ||
| } | ||
|
|
||
| _, han := interceptors.NRHttpTracer("", h.ServeHTTP) | ||
|
|
@@ -268,12 +289,15 @@ func tracingWrapper(h http.Handler) http.Handler { | |
|
|
||
| // spanRouteMiddleware is a grpc-gateway middleware that updates the OTEL span | ||
| // name and http.route attribute with the matched route pattern after routing. | ||
| // It uses runtime.HTTPPattern (the Pattern struct set by handleHandler) rather | ||
| // than runtime.HTTPPathPattern (the string set later inside AnnotateContext). | ||
| func spanRouteMiddleware(next runtime.HandlerFunc) runtime.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request, pathParams map[string]string) { | ||
| if pattern, ok := runtime.HTTPPathPattern(r.Context()); ok { | ||
| if pattern, ok := runtime.HTTPPattern(r.Context()); ok { | ||
| route := pattern.String() | ||
| span := oteltrace.SpanFromContext(r.Context()) | ||
| span.SetName(r.Method + " " + pattern) | ||
| span.SetAttributes(semconv.HTTPRoute(pattern)) | ||
| span.SetName(r.Method + " " + route) | ||
| span.SetAttributes(semconv.HTTPRoute(route)) | ||
| } | ||
| next(w, r, pathParams) | ||
| } | ||
|
|
||
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.
httpSpanAttributesis documented as "omitting empty-valued attributes", but the new logic always appendsurl.scheme(defaulting to "http"). Either update the function comment to match the new behavior, or only seturl.schemewhen it’s explicitly known (URL scheme provided, TLS present, or a trusted forwarded-proto header).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.
Good catch on the comment —
url.schemeis now always set (derived fromX-Forwarded-Proto/r.TLS/ defaulthttp). The function comment should be updated. Will fix if we do another pass on this PR.