feat(coderd/tracing): correlate request logs and spans by session_id - #27671

@jeremyruppel I just re-read the RFC; initial functional requirement 1 is:

Clients must generate 16-byte session ID encoded as a 32-character hexadecimal string.

UUIDs don't conform--they include hyphens in addition to hexadecimal chars. So httpmw's validSessionID function (added in this PR) will return false for a UUID.

I believe we will need a helper function to generate session IDs for the template builder after all, like #27661, just without the hyphens

see #27935

P2 The session_id correlation key is three independent string literals with nothing keeping them in sync. (Structural Analyst P2, Go Architect P3, Duplication Checker Obs, Security Reviewer Obs, Style Reviewer Nit)

SessionIDBaggageKey = "session_id" is the const (line 25), but the log field (slog.F("session_id", sessionID), line 57) and the span attribute (attribute.String("session_id", sessionID), line 71) are hardcoded string literals. If a future editor renames one literal, correlation silently breaks with no test failure (the tests also hardcode "session_id" in one path).

The whole point of the feature is that the baggage key, log field, and span attribute all read session_id so operators can join logs and traces. Cheapest fix that makes the coupling explicit: a second unexported const (e.g. sessionIDField = "session_id") referenced by both the slog.F and attribute.String calls. Not blocking, but worth doing while it's fresh.

P3 validSessionID accepts upper-case hex, which can fragment correlation. (Edge Case Analyst P3, Contract Auditor P3, Structural Analyst P3, Security Reviewer Obs)

validSessionID treats a-f and A-F as valid, and sessionIDFromHeaders returns the raw member value with no case normalization. If any producer of these IDs emits lowercase (Go's %x, hex.EncodeToString, and trace.TraceID.String() all emit lowercase) while another client sends uppercase, exact-string correlation across logs/spans/other subsystems silently fails to join records that are semantically the same session.

The doc comment says the function enforces "the encoding the RFC mandates," which overstates it if mixed case is accepted. Either normalize with strings.ToLower before returning from sessionIDFromHeaders, or tighten the validator to the RFC's canonical case, and reconcile the comment. TestValidSessionID explicitly asserts UpperHex is valid, so please confirm the intended casing against the RFC.

@code-asher My gut says we should accept lowercase hex only, wdyt?

Nit validSessionID hand-rolls a hex check that encoding/hex already covers. (Duplication Checker, Modernization Reviewer)

The repo already validates hex strings with encoding/hex.DecodeString in several places, e.g. coderd/cryptokeys/cache.go, enterprise/coderd/appearance.go (validateHexColor), and codersdk/x/agenthooks/jwt.go. The same result is achievable with b, err := hex.DecodeString(s); return err == nil && len(b) == 16.

Optional and marginal: the current loop is correct and allocation-free, whereas hex.DecodeString allocates a 16-byte buffer per matched request. Note it also accepts upper-case, so it does not resolve the casing concern above. Only worth switching alongside that fix, for consistency with the codebase convention.

Obs The session_id field name collides with an existing, differently-typed session_id concept. (Contract Auditor, Structural Analyst, Edge Case Analyst)

session_id is already an established field name with a different format and meaning in other subsystems, e.g. agentapi/boundary_logs.go logs slog.F("session_id", sessionID.String()) where it is a UUID. This PR introduces a third meaning: a 32-char hex "connection-log RFC" correlation ID.

slog.With appends rather than replaces, so if a future handler on the /api/** path logs its own UUID session_id, one log line carries two session_id fields with different semantics. No collision on that path today, but a more specific name (e.g. connection_session_id) would keep the signal honest.

Obs session_id is unauthenticated, client-controlled input, logged before auth runs. (Security Reviewer, Contract Auditor)

This middleware sits high in the stack, so the field is attached to the request log context before any authentication/authorization middleware executes. A client can forge or reuse any 32-hex value, so two unrelated actors can deliberately collide on the same session_id to muddy an investigation, and the field appears in logs regardless of auth outcome.

Inherent to a baggage-propagated correlation ID and presumably fine per the RFC. Worth a note in the connection-log RFC / operator docs that session_id in logs is client-asserted, not a trustworthy identity signal. The strict hex validation correctly prevents log/attribute injection from this value.

Obs Baggage is now parsed on the tracing-disabled path, and twice when tracing is enabled. (Performance Analyst, Go Architect)

Before this change, when tracer == nil the middleware returned immediately after the route match with zero extra work. Now every matched request first calls sessionIDFromHeaders(r.Header). When tracer != nil, StartHTTPSpan calls otel.GetTextMapPropagator().Extract(...), which parses the same baggage header again.

Negligible for the common no-baggage-header case (one header-map miss, no allocation) and bounded by otel's W3C limits otherwise, so not a blocker. The duplicate parse is avoidable by reading the member off the context StartHTTPSpan produces, at the cost of coupling to the global propagator, which the code intentionally avoids per the function comment. Flagging so the tradeoff stays conscious.

P3 No test covers the non-matching-route path this change reordered. (Test Auditor)

Middleware now checks re.MatchString before extracting the session_id, so extraction/logging is intentionally skipped on unmatched routes. Every Test_Middleware_SessionID subtest uses /api/v2/workspaces, which always matches. A regression that moved extraction back above the route check (logging session_id on static/asset routes, i.e. leaking client-controlled baggage into logs for every request) would not be caught.

Add a subtest that sends baggage to a non-matching path (e.g. /index.html) and asserts session_id is absent from both the log fields and the span. Cheap insurance against a client-baggage log leak.

P3 The MalformedBaggage span assertion is weaker than intended. (Test Auditor)

the subtest asserts require.NotContains(tp.span.attributes(), attribute.String("session_id", "not-a-valid-session-id")). That only proves the specific malformed string wasn't set. A bug that set session_id to some other derived value would still pass. The real invariant is "no session_id attribute is set at all".

Scan tp.span.attributes() for any attribute whose Key == "session_id" and assert its absence, so the test resists that class of bug.

Obs recordingTracer largely duplicates the existing fakeTracer, and there's no tracing-enabled + no-baggage case. (Duplication Checker, Test Auditor)

fakeTracer already implements TracerProvider/Tracer returning a span; recordingTracer re-implements the identical Tracer/Start plumbing, differing only in that it returns a recordingSpan.

Test-only scaffolding, so low cost; the existing fake could have been extended to optionally hand out a recording span. Separately, a recording-tracer + no-baggage subtest would directly pin the "don't set an empty span attribute when sessionID is empty" branch that the other subtests only cover indirectly.