feat: setup logging, tracing and metrics in standalone AI Gateway - #27068
Conversation
Docs preview📖 View docs preview for |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
3ecfb93 to
2d9f138
Compare
pawbana
changed the title
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 13 findings (6 P3, 1 P4, 6 Nit), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel. 6 P3, 5 Nit, 2 Note dropped. CRF-1 not posted (outside diff). Reviewed against f378478..2d9f138. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean extraction of StartHTTPSpan from the coderd middleware, well-designed option inheritance mechanism (group + env + exclusion), and good test coverage for the option surface. The approach of parameterizing existing infrastructure (ConfigureTraceProvider, clilog, ServeHandler) instead of building parallel stacks is exactly right.
6 P3, 5 Nit. No blockers.
Three reviewers independently flagged health endpoint tracing (CRF-2) as the sharpest finding. Gon audited all 13 comments in the diff and found a consistent pattern of restating what the code shows before reaching the useful "why"; the two most impactful are inline, the rest share the same pattern.
The PR description ("Adds logging, tracing and metrics setup") would benefit from noting: (1) why option inheritance uses two mechanisms (group match vs env-var cherry-pick), (2) the CODER_AI_GATEWAY_VERBOSE removal and its migration path, and (3) why the Gateway needs its own tracingMiddleware instead of the shared one.
"An operator with CODER_AI_GATEWAY_VERBOSE=true in their deployment environment starts the Gateway, gets no error, and runs without debug logging. They discover it when they need to debug a production issue and can't see debug output." (Hisoka)
cli/server.go:2798
P3 [CRF-7] This exported function now has 4 parameters and 3 return values, with 3 callers across 3 packages. The second return value is a SQL driver name (not obvious from the type string). The close function's contract (must defer, blocks, has a timeout?) is implicit.
// ConfigureTraceProvider sets up an OpenTelemetry TracerProvider based on
// the deployment's tracing configuration. serviceName identifies the service
// in exported traces (e.g. "coderd", "coder-ai-gateway"). Returns the
// TracerProvider, the SQL driver name for traced database connections, and a
// close function that flushes and shuts down the exporter.(Leorio)
🤖
scaletest/llmmock/server.go:680
P4 [CRF-1] scaletest/llmmock/server.go:680-703 inlines the same trace context extraction, span creation, and response header injection that coderd/tracing.StartHTTPSpan now provides. The PR extracted this logic into a reusable function but did not update the existing duplicate. Could drop ~15 lines by calling StartHTTPSpan.
(Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
2d9f138 to
4d6d9e4
Compare
f378478 to
8d475a7
Compare
4d6d9e4 to
e78c4ce
Compare
e78c4ce to
d4c952a
Compare
Documentation CheckUpdates Needed
Notes
Automated review via Coder Agents |
| traceCloseErr := shutdownWithTimeout(closeTracing, 5*time.Second) | ||
| logger.Debug(signalCtx, "tracing closed", slog.Error(traceCloseErr)) | ||
| }() | ||
| tracer := tracerProvider.Tracer("aibridged") |
There was a problem hiding this comment.
Nit: shouldn't we use the new name here?
There was a problem hiding this comment.
Good catch, cleaned up both tracer and logger names.
| // to a standalone Gateway. CODER_ENABLE_TERRAFORM_DEBUG_MODE is grouped under | ||
| // Logging but controls provisioner behavior that coderd owns. | ||
| excludedEnvs := map[string]struct{}{ | ||
| "CODER_ENABLE_TERRAFORM_DEBUG_MODE": {}, |
There was a problem hiding this comment.
This is just a concern for docs generation, correct?
There was a problem hiding this comment.
Yes, this is for doc and --help message.
I've made option inheritance more straight forward and changed test.
Start command has hardcoded list of inherited optionsaiGatewayInheritedEnvs and test has hardcoded dropped list. Test verifies that every option from inherited groups is in one of those lists.
When new option is added to any group that standalone Gateway inherits options from (Logging, Tracing, AI Gateway, Prometheus) it must be added either to aiGatewayInheritedEnvs or dropped list otherwise test will fail. Maybe it is a bit annoying but it makes sure there won't be any drift.
2 participants
Adds logging, tracing and metrics setup to standalone AI Gateway.
Existing options are re-used when possible.