From 775091b2c678433e6d075134a71193b6679b4823 Mon Sep 17 00:00:00 2001 From: Robert Lin Date: Fri, 9 Jun 2023 11:33:11 -0700 Subject: [PATCH] instrumentation: set X-Trace to trace ID, X-Trace-URL to trace URL, add X-Trace-Span (#53259) This allows us to return `X-Trace` and `X-Trace-Span` from Cody Gateway with the shared HTTP instrumentation helper, which will help linking up traces in case GCP tracing doesn't work out, and also help with debugging customer instances that talk to Cody Gateway. Outside of Sourcegraph core we don't have access to conf so setting this to a URL doesn't make as much sense, and there seems to be a somewhat pre-existing standard in Jaeger to set `x-trace` to a trace ID (?), and `X-Trace-Span` just tacks onto that with a bit more detail (for Cody Gateway locally, `x-trace` will point to the entire trace, would be nice to have the span ID as well). In Sourcegraph core, this means that we now can't set the URL to `X-Trace` as we've done before. Instead, we now set it to `X-Trace-URL`, and send both in responses. There appears to be no references to `X-Trace` that expects an URL, so I think we should be good here - streaming search uses a field on its responses. Related: #53025 ## Test plan With `?trace=1` in both Cody chat and search, I now get something like the following: image The "View trace" still works as expected, and the link and the headers point to the same trace. --- CHANGELOG.md | 1 + doc/admin/observability/tracing.md | 2 +- internal/instrumentation/http.go | 25 +++++++++++++++++++++++-- internal/trace/httptrace.go | 5 ++++- 4 files changed, 29 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 019dac357de..3f62f57823b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ All notable changes to Sourcegraph are documented in this file. been marked as deprecated and will be removed in a future release [#52566](https://github.com/sourcegraph/sourcegraph/pull/52566) - Update minimum supported Redis version to 6.2 [#52248](https://github.com/sourcegraph/sourcegraph/pull/52248) - The batch spec properties [`transformChanges`](https://docs.sourcegraph.com/batch_changes/references/batch_spec_yaml_reference#transformchanges) and [`workspaces`](https://docs.sourcegraph.com/batch_changes/references/batch_spec_yaml_reference#workspaces) are now generally available. +- If a Sourcegraph request is traced, its trace ID and span ID are now set to the `X-Trace` and `X-Trace-Span` response headers respectively. The trace URL (if a template is configured in `observability.tracing.urlTemplate`) is now set to `X-Trace-URL` - previously, the URL was set to `X-Trace`. [#53259](https://github.com/sourcegraph/sourcegraph/pull/53259) ### Fixed diff --git a/doc/admin/observability/tracing.md b/doc/admin/observability/tracing.md index 6203fe212e3..bc21243ff33 100644 --- a/doc/admin/observability/tracing.md +++ b/doc/admin/observability/tracing.md @@ -47,7 +47,7 @@ Note that getting a trace URL requires `urlTemplate` to be configured. ### Trace a GraphQL request To receive a traceID on a GraphQL request, include the header `X-Sourcegraph-Should-Trace: true` with the request. -The response headers of the response will now include an `x-trace` entry, which will have a URL the [exported trace](#tracing-backends). +The response headers of the response will now include an `x-trace-url` entry, which will have a URL the [exported trace](#tracing-backends). Note that getting a trace URL requires `urlTemplate` to be configured. diff --git a/internal/instrumentation/http.go b/internal/instrumentation/http.go index 0da1ca0c8d2..c69310b5b2e 100644 --- a/internal/instrumentation/http.go +++ b/internal/instrumentation/http.go @@ -52,11 +52,32 @@ var defaultOTELHTTPOptions = []otelhttp.Option{ // using the globally configured propagator. // // The provided operation name is used to add details to spans. -func HTTPMiddleware(operation string, h http.Handler, opts ...otelhttp.Option) http.Handler { - instrumentedHandler := otelhttp.NewHandler(h, operation, +func HTTPMiddleware(operation string, next http.Handler, opts ...otelhttp.Option) http.Handler { + afterInstrumentedHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Set X-Trace after otelhttp's handler which starts the trace. The + // top-level trace should be an OTEL trace, so we use otel/trace to + // extract it. Then, we add it to the header before next writes the + // header back to client. + span := trace.SpanContextFromContext(r.Context()) + if span.IsValid() { + // We only set the trace ID here. The trace URL is set to + // X-Trace-URL by httptrace.HTTPMiddleware that does some more + // elaborate handling. In particular, we don't want to introduce + // a conf.Get() dependency here to build the trace URL, since we + // want this to be fairly bare-bones for use in standalone services + // like Cody Gateway. + w.Header().Set("X-Trace", span.TraceID().String()) + w.Header().Set("X-Trace-Span", span.SpanID().String()) + } + + next.ServeHTTP(w, r) + }) + + instrumentedHandler := otelhttp.NewHandler(afterInstrumentedHandler, operation, append(defaultOTELHTTPOptions, opts...)...) return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Set up trace policy before instrumented handler var shouldTrace bool switch policy.GetTracePolicy() { case policy.TraceSelective: diff --git a/internal/trace/httptrace.go b/internal/trace/httptrace.go index bc245215f65..c88d335ce28 100644 --- a/internal/trace/httptrace.go +++ b/internal/trace/httptrace.go @@ -122,8 +122,11 @@ func HTTPMiddleware(l log.Logger, next http.Handler, siteConfig conftypes.SiteCo trace := Context(ctx) var traceURL string if trace.TraceID != "" { + // We set X-Trace-URL to a configured URL template for traces. + // X-Trace for the trace ID is set in instrumentation.HTTPMiddleware, + // which is a more bare-bones OpenTelemetry handler. traceURL = URL(trace.TraceID, siteConfig) - rw.Header().Set("X-Trace", traceURL) + rw.Header().Set("X-Trace-URL", traceURL) logger = logger.WithTrace(trace) }