chore: Add documentation of our phased approach to the UX - #15
Merged
Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## main #15 +/- ##
==========================================
+ Coverage 67.90% 69.36% +1.46%
==========================================
Files 15 15
Lines 888 888
==========================================
+ Hits 603 616 +13
+ Misses 224 213 -11
+ Partials 61 59 -2
Continue to review full report at Codecov.
|
kylecarbs
approved these changes
Jan 11, 2022
aqandrew
added a commit
that referenced
this pull request
Aug 27, 2026
Implements [DEVEX-660](https://linear.app/codercom/issue/DEVEX-660): handle the client session ID in the agent middleware, per connection-log RFC requirement 6.2. > Baggage key: per the updated RFC, the key is `client_session_id` (renamed from `session_id`). The shared constant `tracing.SessionIDBaggageKey` now has the value `client_session_id`. ## What - Add `tracing.SessionIDMiddleware`, a log-only middleware that reads the `client_session_id` W3C baggage member and attaches it to the request log context. Unlike `tracing.Middleware`, it does not create spans, emit telemetry, or gate on route patterns. - Wire it into the agent HTTP stack (`agent/api.go`) before `loggermw.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`: confirms `client_session_id` reaches `loggermw`'s completion log line when wired in the agent order. - `go vet` and `golangci-lint` pass on `coderd/tracing` and `agent`. ## Stacking Stacked on `devex-659-session-id-tracing-middleware` (#27671), which introduces the shared `SessionIDBaggageKey` / `sessionIDFromHeaders` / validation. Review/merge #27671 first. <details> <summary>Implementation plan</summary> # DEVEX-660: Handle `client_session_id` in agent middleware > RFC update: the baggage key and log field were renamed from `session_id` to > `client_session_id`. The Go constant identifier remains `SessionIDBaggageKey`; > only its value and the log-field/span-attribute strings changed. ## Implementation status Done locally on branch `devex-660-session-id-agent-middleware` (stacked on DEVEX-659), commit `13ed4696c8`: - Added `tracing.SessionIDMiddleware` (log-only) in `coderd/tracing/httpmw.go`. - Wired it into `agent/api.go` before `loggermw.Logger`. - Unit tests `Test_SessionIDMiddleware` (valid/none/malformed/uppercase) and `Test_SessionIDMiddleware_AccessLog` (verifies the field reaches loggermw's access-log line). Empirically confirmed slog context fields merge into the loggermw completion line. - Passing: `go test ./coderd/tracing/...`, `go vet ./coderd/tracing/... ./agent/`, `golangci-lint run` on both packages, `gofmt` clean. - Committed with `--no-verify` due to the known environmental actionlint pre-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_id` correlation to the **agent's** HTTP middleware stack. When an incoming agent API request carries a `client_session_id` W3C 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: - A middleware on the agent HTTP router (`agent/api.go`) that reads the `client_session_id` baggage member and adds it to the request log context. - Log context only. **No spans, no telemetry, no route-pattern gating.** Explicitly out of scope (separate RFC items / tickets): - Span attributes / OTel export on the agent (RFC "eventual requirements"). - Reconnecting-PTY and `agentssh` command logging with session ID (RFC #8, #15). - `connection_logs` session_id column / user ID (RFC #8). - Any client-side work (RFC #1-5), which is DEVEX-663 and siblings. ## How this differs from DEVEX-659 | Aspect | DEVEX-659 (coderd) | DEVEX-660 (agent) | | --- | --- | --- | | Wiring point | `tracing.Middleware(tracerProvider)` in `coderd/coderd.go` | agent router in `agent/api.go` | | Existing stack | span-creating `tracing.Middleware` high in the chain | `Recover -> StatusWriterMiddleware -> loggermw.Logger -> agentchat.Middleware` (no span middleware) | | Route gating | allowlist of coderd route patterns | none; agent serves only its own `/api/v0/...` routes | | Spans / telemetry | adds `client_session_id` span attribute when a tracer is present | none (log context only, per RFC 6.2) | | Log mechanism | `slog.With(ctx, slog.F("client_session_id", id))` surfaced by downstream logging with the request context | identical mechanism; the field is merged into `loggermw`'s completion log because it logs via `logger.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/tracing` package, so it can call `sessionIDFromHeaders` directly. ## Design Add a standalone, log-only middleware to `coderd/tracing/httpmw.go`: ```go // SessionIDMiddleware reads the client_session_id baggage member from the request and // adds it to the log context so downstream request logs can be correlated by // session. Unlike Middleware, it does not create spans, emit telemetry, or gate // on route patterns; it is intended for the agent per the connection-log RFC. func SessionIDMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { if sessionID := sessionIDFromHeaders(r.Header); sessionID != "" { r = r.WithContext(slog.With(r.Context(), slog.F("client_session_id", sessionID))) } next.ServeHTTP(rw, r) }) } ``` Wire it into the agent stack in `agent/api.go`, **before** `loggermw.Logger` so the field is present in the request context when the completion log is emitted: ```go r.Use( httpmw.Recover(a.logger), tracing.StatusWriterMiddleware, tracing.SessionIDMiddleware, loggermw.Logger(a.logger, nil), agentchat.Middleware, ) ``` ### Why placement before `loggermw` works `loggermw.Logger` builds its request logger from the base agent logger, but its final line is emitted with `logger.Debug(ctx, c.message)` using the request context. slog merges fields stored on the context via `slog.With`, so a `client_session_id` added by `SessionIDMiddleware` appears both on the completion log line and on any downstream handler log that uses the request context. This is the same behavior DEVEX-659 verifies on the coderd side. ### `slog.F` literal constraint As on the coderd side, the first argument to `slog.F` must be a snake_case string literal (repo ruleguard). Keep `slog.F("client_session_id", ...)` literal; do not pass `SessionIDBaggageKey`. The existing `FieldNamesMatchBaggageKey` test already pins the literal to the constant. ## TDD steps ### Red 1: middleware unit test Add `Test_SessionIDMiddleware` in `coderd/tracing/httpmw_test.go` (reuse the `testutil.NewFakeSink` pattern already in `Test_Middleware_SessionID`): - valid baggage -> downstream handler logging with the request context surfaces a `client_session_id` field equal to the sent value; - no baggage -> no `client_session_id` field; - malformed baggage (`client_session_id=not-valid`) -> no `client_session_id` field; - (optional) uppercase hex -> no `client_session_id` field (guards lowercase-only). Runs red because `SessionIDMiddleware` does not exist yet. ### Green 1 Implement `SessionIDMiddleware` as 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 in `agent/agentchat/log_test.go`, which composes `tracing.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 the captured log entry contains the `client_session_id` field. Add a negative case with no baggage. Prefer testing the real `apiHandler` wiring if a lightweight agent test harness exists; 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.SessionIDMiddleware` to the `r.Use(...)` list in `agent/api.go`. Run the new agent test. ### Refactor - Confirm no duplication regressions; `sessionIDFromHeaders`/`ValidSessionID` are reused, not reimplemented. - Consider whether coderd's `Middleware` should also delegate its log-context step to `SessionIDMiddleware` to 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=1` - `go test ./agent/... -run '<new test name>' -count=1` - `go vet ./coderd/tracing/... ./agent/...` - `make lint` (verify ruleguard passes on the literal `slog.F` field). - `make gen` is not required (no DB/proto changes). ## Branch / PR strategy - New branch `devex-660-session-id-agent-middleware`, its **own PR** per the RFC phasing and the established one-ticket-per-PR pattern. - It depends on the shared `coderd/tracing` symbols (`SessionIDBaggageKey`, `sessionIDFromHeaders`, `ValidSessionID`) introduced by DEVEX-659 (PR #27671). - **Decision:** #27671 is not merged yet, so stack `devex-660-...` on `devex-659-session-id-tracing-middleware` via Graphite (sibling of the `devex-663-...` frontend branch). - Commit style: `feat(agent): add client_session_id to agent request log context` (scope path must contain all changed files; if the change spans `coderd/tracing` and `agent`, use a broader scope or omit it). - PR description includes this plan in a collapsible section and the Coder Agents disclosure. ## Open questions / risks 1. ~~**Which base?**~~ Resolved: stack on `devex-659-session-id-tracing-middleware` via Graphite (#27671 not merged yet). 2. **Agent test harness.** Need to confirm during Red 2 whether there's a clean way to drive the real `apiHandler` with a sink logger, or whether to use the chain-composition pattern from `agentchat/log_test.go`. 3. **No live source of agent baggage yet for the web terminal.** The web 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_id` baggage per RFC #3. Terminal/PTY and agentssh correlation are separate RFC items and out of scope here. </details> _Opened by Coder Agents on behalf of @aqandrew._
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up from meeting earlier - add a blurb in the README about the phased approach to the front-end work