feat: add per-request trace IDs and system-error metrics - #2849
feat: add per-request trace IDs and system-error metrics#2849charleswool wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (42.85%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2849 +/- ##
==========================================
- Coverage 76.89% 76.86% -0.04%
==========================================
Files 90 90
Lines 4276 4279 +3
==========================================
+ Hits 3288 3289 +1
- Misses 831 832 +1
- Partials 157 158 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds request-scoped observability to the Gatekeeper provider request path by initializing per-request logging context (trace IDs) and expanding metrics reporting for request durations and handler/system errors.
Changes:
- Initialize request context in
verify/mutateHTTP handlers vialogger.InitContextto enable per-request trace IDs in downstream context-aware logs. - Add request duration reporting for
verify/mutateand report handler/system errors viametrics.ReportSystemError. - Wire metrics exporter initialization into the Gatekeeper provider binary behind a new
--metrics-portflag, with accompanying test updates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/httpserver/server.go | Initializes request context for verify/mutate handlers to propagate trace IDs. |
| internal/httpserver/handlers.go | Adds request-duration metrics, context-aware cache warning logs, and system-error metrics reporting. |
| cmd/ratify-gatekeeper-provider/main.go | Adds metrics exporter initialization via --metrics-port. |
| cmd/ratify-gatekeeper-provider/main_test.go | Updates tests for metrics init wiring and default flag parsing. |
Comments suppressed due to low confidence (2)
internal/httpserver/handlers.go:122
metrics.ReportSystemErrorrecords the provided string as a metric attribute. Using the full parse error message (item.Error) will produce unbounded label cardinality (different user inputs / error details). Use a stable category (the detailed error is already returned to the caller viaitem.Error).
metrics.ReportSystemError(ctx, item.Error)
internal/httpserver/handlers.go:159
metrics.ReportSystemErroruses the string as a metric label; sendingerr.Error()here can produce high-cardinality metrics (downstream error text often varies). Use a stable category string instead and keep the full error in logs/response payloads.
metrics.ReportSystemError(ctx, err.Error())
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s *server) verifyHandler() http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| _ = s.verify(r.Context(), w, r) | ||
| _ = s.verify(logger.InitContext(r.Context(), r), w, r) |
There was a problem hiding this comment.
Correct - InitLogConfig is not called in this binary, so trace IDs are generated per request, not read from incoming headers. I have fixed the PR description to say generated; header-based propagation can be wired later by calling InitLogConfig with the header names.
| }) | ||
| if err != nil { | ||
| results[idx].Error = err.Error() | ||
| metrics.ReportSystemError(ctx, err.Error()) |
There was a problem hiding this comment.
Good catch - switched ReportSystemError to stable category labels (verify_artifact, mutate_parse_reference, mutate_resolve_reference) to keep Prometheus label cardinality bounded. The full error text still goes into the response item returned to the caller.
| name: "metrics init failure is non-fatal", | ||
| opts: &options{ | ||
| httpServerAddress: ":8080", | ||
| configFilePath: "config.yaml", | ||
| certFile: "cert.pem", | ||
| disableCertRotation: true, | ||
| disableCRDManager: true, | ||
| metricsPort: 8888, | ||
| }, | ||
| expectError: true, |
There was a problem hiding this comment.
Addressed in the metrics PR this is stacked on (#2847): the non-fatal case now uses a sentinel error and asserts via errors.Is that startRatify does not return the metrics-init error. This branch was rebuilt on that updated commit.
7dbfaf2 to
fdfb59a
Compare
fdfb59a to
157b8e8
Compare
Attach a trace ID to each verify/mutate request context via logger.InitContext, so it propagates to all context-aware downstream logging (executor, verifiers, auth and policy providers). Switch the handler cache-warning logs to the context logger so they carry the trace ID too, and count handler/processing failures with metrics.ReportSystemError using stable low-cardinality category labels (verify_artifact, mutate_parse_reference, mutate_resolve_reference). Signed-off-by: Charles Wu <yuewu2@microsoft.com>
157b8e8 to
224baea
Compare
Description
Improves log correlation and error observability for the provider's request path.
Now rebased directly onto
main— the metrics wiring it depended on landed with #2848, and the now-empty #2847 was closed. This PR is a single commit againstmain.Change
verify/mutaterequest context vialogger.InitContext. It propagates to all context-aware downstream logging — executor, verifiers, auth providers, policy providers — so a single request's logs correlate. (Reading the trace ID from an incoming request header is supported byinternal/loggerbut requiresInitLogConfigto register the header names, which this binary does not call today; that can be wired up in a follow-up.)metrics.ReportSystemError, using stable, low-cardinality category labels (verify_artifact,mutate_parse_reference,mutate_resolve_reference) so Prometheus label cardinality stays bounded. The full error text is still returned to the caller in the response item.Further metric-coverage expansion (outcome labels on request histograms, cache hit/miss and registry/downstream counters) is left as a follow-up.
#2873 builds on this branch and adds request-outcome logging.
Testing
go build ./...,go vet, package tests, andgolangci-lintpass.ReportSystemErroris nil-guarded so it is a no-op when metrics are disabled.