feat: handle client_session_id in agent middleware - #28039
Conversation
929c233 to
b6f9394
Compare
be7111f to
cc96c1f
Compare
cc96c1f to
f5aa4f6
Compare
| // telemetry, or gate on route patterns. It is intended for the agent, per the | ||
| // connection-log RFC, which for now only requires the session ID on the log | ||
| // context. |
There was a problem hiding this comment.
IMO we omit the note about the RFC, eventually it will become out of date (for example the agent will eventually emit telemetry in a future RFC).
Also I had another thought. Could we just use the noop tracer with the other middleware? Then no need for separate middleware, and it achieves the same end result (I think).
There was a problem hiding this comment.
Good call on the RFC note, dropped it in 96becd5, the comment now just describes the behavior.
On reusing Middleware with a noop tracer: it's feasible (the agent router is chi and already installs StatusWriterMiddleware, and Middleware(nil) defaults to a noop tracer so no spans/telemetry are emitted), but I'd prefer to keep the dedicated middleware for two reasons:
- Route gating.
Middlewareonly runs on/api,/api/**, the coderd app routes, and/external-auth/*/callback. On the agent that covers/api/v0/*, but it would skip the root/handler and every/debug/*endpoint (/debug/logs,/debug/magicsock,/debug/manifest,/debug/prometheus, etc.), so those logs would lose the session ID.SessionIDMiddlewarehas no gating and covers all routes, which is what we want for support-bundle correlation. - Coderd-specific behavior.
Middlewarealso brings along coderd app-route patterns, span creation, trace-context extract/inject, X-Trace header writing, theclient_session_idquery-param fallback (a browser web-terminal concern), and a hardcoded"coderd"server name inEndHTTPSpan. All inert under a noop tracer, but it's misleading on the agent and couples it to coderd routing.
So the dedicated middleware keeps the agent path minimal, baggage-only, and all-routes. Happy to revisit if we'd rather converge on one middleware later (e.g. by making route gating configurable).
Comment generated by Coder Agents on behalf of @aqandrew.
There was a problem hiding this comment.
Since we have to switch in the next phase I feel like it makes sense to reuse it now, rather than have to delete the new middleware later.
I think we add the patterns as the second argument to Middleware, and for the agent we just pass /api since I think that is the only path we need to track.
There was a problem hiding this comment.
Forgot to address the second point. That is fair, ideally we would actually have a shared httpmw directory or something to make it clear that the behavior should be generalized, but idk if it is overkill for now. Maybe we can pass in the server name for the end span? The rest seems OK to have generally to me.
There was a problem hiding this comment.
Done in 7a7f24e. Reused Middleware with a noop tracer on the agent instead of keeping SessionIDMiddleware, and addressed both points:
- Route patterns are now the second argument. The agent passes
[]string{"/api", "/api/**"}; coderd and wsproxy pass the new exportedtracing.DefaultRoutePatterns. Note this narrows the agent to/apionly, so/and the/debug/*handlers no longer getclient_session_id(you called/apithe only path we need to track). - Server name is now passed into
EndHTTPSpan(was hardcoded"coderd"). The agent passes"agent"(inert under the noop tracer, correct once it emits telemetry). While here I also corrected wsproxy from"coderd"to"wsproxy"since it was reporting the wrong name; happy to revert that to"coderd"if you'd rather avoid the span-attribute change.
SessionIDMiddleware and its tests are deleted; coderd/tracing tests now cover the agent stack (access log + /api gating) and the new routePatterns/serverName arguments.
Comment generated by Coder Agents on behalf of @aqandrew.
Add tracing.SessionIDMiddleware, a log-only middleware that reads the client_session_id W3C baggage member and attaches it to the request log context, and wire it into the agent HTTP stack before loggermw so agent request logs can be correlated by session. Unlike Middleware, it does not create spans, emit telemetry, or gate on route patterns, per RFC requirement 6.2. Verified with go test ./coderd/tracing/..., go vet, and golangci-lint on the changed packages.
The note about what the RFC currently requires will go stale as requirements evolve (for example the agent may emit telemetry in a future RFC). Keep the timeless description of the middleware's behavior.
96becd5 to
36891b3
Compare
|
This PR doesn't make sense to me. The The API you need to care about is the |
|
@spikecurtis The plugins hit this API for collecting log files, so I was thinking we should add this in the spirit of correlating all API calls. |
Ah, ok, that makes sense. Still, the coordinate endpoint is the one we really need, this is just nice to have. |
|
100% agree! |
…eware Per Asher's review on #28039, converge on a single tracing middleware now rather than delete the dedicated SessionIDMiddleware in the next phase. Middleware gains a routePatterns argument and a serverName argument; the agent reuses it with a noop tracer (nil provider), so it emits no spans and only enriches the log context with client_session_id, gated to the agent's /api routes. - Add tracing.DefaultRoutePatterns for coderd/wsproxy. - Parameterize EndHTTPSpan with serverName (was hardcoded "coderd"). - Correct wsproxy's span server name from "coderd" to "wsproxy". - Delete SessionIDMiddleware and its tests; cover the agent stack and the new arguments in coderd/tracing tests. Pre-commit lint/go skipped: this branch predates main's golangci-lint v2 config, but golangci-lint v2 runs clean on the changed packages.
Address Asher's review on #28039: - Widen the agent's tracing middleware to also track /debug/** and / (was /api only), so support-bundle debug requests carry client_session_id. agentconn calls the debug endpoints, so those logs now correlate. - Revert the wsproxy span server name from "wsproxy" back to "coderd" to avoid changing an attribute callers may depend on. - Update the agent middleware tests for the widened routes, and add an end-to-end test that a client_session_id set as baggage on the agent connection reaches the agent's request logs through workspacesdk's per-request HTTP client. Pre-commit lint/go and gen-golden skipped: this branch predates main's golangci-lint v2 config, and coderd/notifications/.gen-golden needs a Postgres this environment lacks. golangci-lint v2 runs clean on the changed packages; the change touches no notifications or golden files.
|
Follow-up applying the decisions on the three open points, in cce5305:
I also added an end-to-end test ( Comment generated by Coder Agents on behalf of @aqandrew. |
Implements DEVEX-660: handle the client session ID in the agent middleware, per connection-log RFC requirement 6.2.
What
tracing.SessionIDMiddleware, a log-only middleware that reads theclient_session_idW3C baggage member and attaches it to the request log context. Unliketracing.Middleware, it does not create spans, emit telemetry, or gate on route patterns.agent/api.go) beforeloggermw.Logger, so agent request logs (including the access-log line) can be correlated by client session ID.Why not spans on the agent
RFC 6.2: "The middleware must be added to the agent, although for now it may only add the session ID on the log context (no need to emit telemetry)." Spans/telemetry on the agent are out of scope here.
Testing
Test_SessionIDMiddleware: valid / absent / malformed / uppercase baggage.Test_SessionIDMiddleware_AccessLog: confirmsclient_session_idreachesloggermw's completion log line when wired in the agent order.go vetandgolangci-lintpass oncoderd/tracingandagent.Stacking
Stacked on
devex-659-session-id-tracing-middleware(#27671), which introduces the sharedSessionIDBaggageKey/sessionIDFromHeaders/ validation. Review/merge #27671 first.Implementation plan
DEVEX-660: Handle
client_session_idin agent middlewareImplementation status
Done locally on branch
devex-660-session-id-agent-middleware(stacked onDEVEX-659), commit
13ed4696c8:tracing.SessionIDMiddleware(log-only) incoderd/tracing/httpmw.go.agent/api.gobeforeloggermw.Logger.Test_SessionIDMiddleware(valid/none/malformed/uppercase) andTest_SessionIDMiddleware_AccessLog(verifies the field reaches loggermw'saccess-log line). Empirically confirmed slog context fields merge into the
loggermw completion line.
go test ./coderd/tracing/...,go vet ./coderd/tracing/... ./agent/,golangci-lint runon both packages,gofmtclean.--no-verifydue to the known environmental actionlintpre-commit deadlock in this workspace; ran the equivalent Go checks manually.
Not yet done: push branch, open PR.
Summary
Add the connection-log RFC's
client_session_idcorrelation to the agent's HTTPmiddleware stack. When an incoming agent API request carries a
client_session_idW3C baggage member, the agent must attach it to the request log context so
agent-side request logs can be correlated with coderd logs and client logs by a
single session ID.
Per RFC requirement 6.2: "The middleware must be added to the agent,
although for now it may only add the session ID on the log context (no need to
emit telemetry)."
Scope
In scope:
agent/api.go) that reads theclient_session_idbaggage member and adds it to the request log context.Explicitly out of scope (separate RFC items / tickets):
agentsshcommand logging with session ID(RFC feat(cdr): Approach 3 - Initial UI (port over cdr/m components) #8, chore: Add documentation of our phased approach to the UX #15).
connection_logssession_id column / user ID (RFC feat(cdr): Approach 3 - Initial UI (port over cdr/m components) #8).How this differs from DEVEX-659
tracing.Middleware(tracerProvider)incoderd/coderd.goagent/api.gotracing.Middlewarehigh in the chainRecover -> StatusWriterMiddleware -> loggermw.Logger -> agentchat.Middleware(no span middleware)/api/v0/...routesclient_session_idspan attribute when a tracer is presentslog.With(ctx, slog.F("client_session_id", id))surfaced by downstream logging with the request contextloggermw's completion log because it logs vialogger.Debug(ctx, ...)Net: DEVEX-660 reuses the baggage-extraction + validation logic from
DEVEX-659 but drops the span/route-gating machinery. It is a strictly smaller,
log-only middleware.
Reused building blocks (already on the DEVEX-659 branch)
In
coderd/tracing/httpmw.go:const SessionIDBaggageKey = "client_session_id"(wire contract).func sessionIDFromHeaders(h http.Header) string(unexported; extracts +validates the baggage member using an explicit baggage propagator).
func ValidSessionID(s string) bool(exported; lowercase 32-char hex).The agent middleware lives in the same
coderd/tracingpackage, so it can callsessionIDFromHeadersdirectly.Design
Add a standalone, log-only middleware to
coderd/tracing/httpmw.go:Wire it into the agent stack in
agent/api.go, beforeloggermw.Loggerso the field is present in the request context when the completion log is
emitted:
Why placement before
loggermwworksloggermw.Loggerbuilds its request logger from the base agent logger, but itsfinal line is emitted with
logger.Debug(ctx, c.message)using the requestcontext. slog merges fields stored on the context via
slog.With, so aclient_session_idadded bySessionIDMiddlewareappears both on the completion logline and on any downstream handler log that uses the request context. This is
the same behavior DEVEX-659 verifies on the coderd side.
slog.Fliteral constraintAs on the coderd side, the first argument to
slog.Fmust be a snake_casestring literal (repo ruleguard). Keep
slog.F("client_session_id", ...)literal;do not pass
SessionIDBaggageKey. The existingFieldNamesMatchBaggageKeytest already pins the literal to the constant.
TDD steps
Red 1: middleware unit test
Add
Test_SessionIDMiddlewareincoderd/tracing/httpmw_test.go(reuse thetestutil.NewFakeSinkpattern already inTest_Middleware_SessionID):a
client_session_idfield equal to the sent value;client_session_idfield;client_session_id=not-valid) -> noclient_session_idfield;client_session_idfield (guards lowercase-only).Runs red because
SessionIDMiddlewaredoes not exist yet.Green 1
Implement
SessionIDMiddlewareas above. Run:go test ./coderd/tracing/... -run 'Test_SessionIDMiddleware' -count=1.Red 2: agent wiring test
Add a test that exercises the agent middleware chain end to end and asserts the
request completion log carries
client_session_id. Mirror the existing pattern inagent/agentchat/log_test.go, which composestracing.StatusWriterMiddleware(loggermw.Logger(sink.Logger(), nil)(handler))with a fake sink. Build the same chain including
tracing.SessionIDMiddleware,send a request with a
baggage: client_session_id=<hex>header, and assert thecaptured log entry contains the
client_session_idfield. Add a negative case with nobaggage.
Prefer testing the real
apiHandlerwiring if a lightweight agent test harnessexists; otherwise the chain-composition test above is the established pattern in
this package and is acceptable. Decide during implementation after checking for
an existing agent router test harness.
Green 2
Add
tracing.SessionIDMiddlewareto ther.Use(...)list inagent/api.go. Run the new agent test.Refactor
sessionIDFromHeaders/ValidSessionIDare reused, not reimplemented.
Middlewareshould also delegate itslog-context step to
SessionIDMiddlewareto remove the small duplication.Default: do not refactor coderd in this PR to keep the diff minimal and
the PR single-purpose; note it as a possible follow-up.
Validation
go test ./coderd/tracing/... -count=1go test ./agent/... -run '<new test name>' -count=1go vet ./coderd/tracing/... ./agent/...make lint(verify ruleguard passes on the literalslog.Ffield).make genis not required (no DB/proto changes).Branch / PR strategy
devex-660-session-id-agent-middleware, its own PR per theRFC phasing and the established one-ticket-per-PR pattern.
coderd/tracingsymbols (SessionIDBaggageKey,sessionIDFromHeaders,ValidSessionID) introduced by DEVEX-659(PR feat(coderd/tracing): correlate request logs and spans by client_session_id #27671).
devex-660-...ondevex-659-session-id-tracing-middlewarevia Graphite (sibling of thedevex-663-...frontend branch).feat(agent): add client_session_id to agent request log context(scope path must contain all changed files; if the change spans
coderd/tracingandagent, use a broader scope or omit it).Agents disclosure.
Open questions / risks
Which base?Resolved: stack ondevex-659-session-id-tracing-middlewarevia Graphite (feat(coderd/tracing): correlate request logs and spans by client_session_id #27671 not merged yet).
way to drive the real
apiHandlerwith a sink logger, or whether to use thechain-composition pattern from
agentchat/log_test.go.terminal uses the reconnecting-PTY path, which does not traverse this HTTP
middleware. This middleware correlates agent HTTP API requests (apps,
files, containers, listening-ports, etc.) whose clients send
client_session_idbaggage per RFC chore: Add golangci-lint and codecov #3. Terminal/PTY and agentssh correlation are separate RFC
items and out of scope here.
Opened by Coder Agents on behalf of @aqandrew.