Repository navigation
Conversation
![github-license-compliance[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
🔵 Needs a closer look
Privacy-sensitive exports, new dependencies and the strict one-second shutdown budget require final human validation.
3 open issues
What changed in this PR
Adds opt-in OpenTelemetry to the Go router while preserving its HTTP and CLI contracts.
Changes:
- Adds OTLP/HTTP configuration, sanitized diagnostics and bounded shutdown flushing.
- Instruments HTTP and planning operations, metrics and request-log trace correlation.
- Adds telemetry tests, documentation and dependency maintenance.
| File | Description |
|---|---|
containers/router/telemetry.go |
Configures telemetry providers, exporters and flushing. |
containers/router/telemetry_server_test.go |
Tests instrumentation, propagation and privacy. |
containers/router/telemetry_export_test.go |
Tests collector exports, failures and shutdown timing. |
containers/router/telemetry_config_test.go |
Tests settings, resources and sanitized diagnostics. |
containers/router/server.go |
Adds request and planning instrumentation. |
containers/router/server_test.go |
Validates telemetry-related log fields. |
containers/router/serve.go |
Initializes telemetry and flushes on exit. |
containers/router/serve_test.go |
Extends environment helpers and startup expectations. |
containers/router/route.go |
Extracts resolved profiles and catalogue lookup. |
containers/router/README.md |
Documents telemetry configuration and behavior. |
containers/router/go.sum |
Records dependency checksums. |
containers/router/go.mod |
Adds OpenTelemetry dependencies. |
containers/router/cli.go |
Passes environment access into serving. |
.github/workflows/test-router.yml |
Enables Go module caching. |
.github/dependabot.yml |
Adds grouped router module updates. |
Review effort: Max
The router exports traces and metrics over OTLP/HTTP when the standard OTEL_* variables configure an endpoint, and stays silent otherwise. Incoming W3C trace context is honoured, request logs gain validated trace_id and span_id fields, spans and metrics carry only allowlisted attributes, and shutdown flushes within the 1 s AWF allows. Raise the indirect golang.org/x/net requirement to v0.60.0 so that govulncheck passes, add a gomod Dependabot entry, and cache Go modules in the router workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add a random service.instance.id to the resource, so that the cumulative metric streams of concurrent routers stay apart at a collector. - Drop http.server.active_requests. It's a Development-status semconv instrument, and a router serving a handful of requests a run has no useful concurrency gauge. Keep gh_aw_router.requests.rejected as a named load-shedding signal. - Shorten the shutdown budget with telemetry on to a 0.25 s drain and a 0.5 s flush. The worst case measured with a stalled collector drops from about 0.92 s to 0.76 s, leaving margin under AWF's 1 s before SIGKILL. - Stop recommending a 15 s metric interval for AWF. Shutdown exports the cumulative totals, so short runs keep them with the default interval. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Don't report an export that shutdown cancels; the final flush reports its own result. Classify a failure with its export context's error too, because the exporters return the SDK context's cause, so a metric reader timeout was reported as "other" instead of "timeout". - Test that OTEL_EXPORTER_OTLP_CERTIFICATE reaches the exporters' TLS client, and that an untrusted collector is reported without detail. - Make the unavailable-collector test observe real export failures before shutdown, and add redirected paths to the cardinality test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
6de90dc to
3e0f64a
Compare
|
✅ Copilot review passed with no inline comments. @rabeckett Add the |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓
|
|
🚀 Security Guard has started processing this pull request |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤
|
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓
|
|
🔌 Smoke Services — All services reachable! ✅
|
|
Smoke Cloud Hypervisor completed. Cloud Hypervisor + Copilot passed.
|
|
✅ Smoke Claude passed
|
|
🛡️ Smoke Copilot Network Isolation confirmed the egress allowlist is enforced. ✅
|
|
✅ Smoke Gemini completed. All facets verified. 💎
|
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅
|
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 Smoke CodexOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓
|
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded.
|
|
✅ Build Test Suite completed successfully!
|
|
Cloud Hypervisor + Copilot smoke test
|
Smoke Test: Claude Engine Validation
Overall result: PASS
|
|
Smoke test (Copilot) — @rabeckett
|
github-actions
Bot
added
smoke-copilot
smoke-copilot-network-isolation
|
EGRESS_RESULT allow=pass deny=pass
Overall: PASS. cc @rabeckett
|
|
Smoke services: ✅ Redis PONG · ✅ pg_isready · ✅ SELECT 1 → PASS
|
|
OTEL smoke test
|
Smoke Test: Copilot BYOK (Direct) Mode ✅ PASSDirect BYOK Mode (COPILOT_PROVIDER_API_KEY via api-proxy → api.githubcopilot.com)
Status: All tests passed. Inference path operational.
|
|
Smoke test:
|
Smoke Test Results
Overall status: FAIL
|
Chroot Version Comparison
Result: Not all tests passed (Node.js version mismatch), so the
|
|
@rabeckett
|
|
@rabeckett Smoke test under direct BYOK (COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw):
Overall: FAIL
|
🏗️ Build Test Suite Results
Overall: 5/8 ecosystems passed — FAIL Failure details
The
|
Closes #10380
Part of #10261 (step 6 of 7).
Summary
This PR adds opt-in OpenTelemetry traces and metrics to the Go router:
OTEL_*environment variables and exported over OTLP/HTTP with protobuf encoding.requestlog records gaintrace_idandspan_id, and startup logs onetelemetry configuredrecord.AWF doesn't pass any telemetry settings to the router yet, so AWF runs behave as before. Step 7 adds the AWF wiring and trace context on the API proxy's router calls.
Changes
telemetry.go(new):telemetry setting invalid, naming the variable but not its value, and turns off the signals it applies to.service.name,service.version(always the binary's), a randomservice.instance.idfor each process, and onlyservice.namespaceanddeployment.environment.namefromOTEL_RESOURCE_ATTRIBUTES.telemetry errorper signal per minute, with a short reason (the HTTP status,timeout,connection refusedorother) and never the exporter's error text.server.go:traceparentandtracestate, and ignoresbaggage.SERVERspan for each request, exceptGET /healthzand a bare 503 from the 64-request limit.INTERNALchild span for/classifyand/routeonce the body decodes.requestrecord.route.go:resolvedProfile, so spans and metrics can record the mode a route was served with.cataloguePair, soselected.modelandselected.effortalways come from the loaded catalogue, never from the request.serve.go,cli.go:telemetry_config_test.go: configuration, precedence, warnings that never contain the value, the resource and sampling.telemetry_server_test.go: spans, metrics, log fields, a cardinality bound (including redirected paths), and privacy canaries.telemetry_export_test.go: end-to-end export to a fake collector that decodes the protobuf, disabled telemetry, an HTTPS collector with and without its CA configured, and unavailable, failing or stalled collectors. It also checks that shutdown stays within 1 s.go.mod,go.sum:otel,sdk,sdk/metric,otlptracehttp,otlpmetrichttp) and semconv v1.43.0.golang.org/x/netv0.60.0 (see the decisions).test-router.ymlturns on setup-go's module cache, keyed oncontainers/router/go.sum, now that the module has dependencies.dependabot.ymlgets a weekly groupedgomodentry for/containers/router.What's emitted
Spans.
SERVERspan per request, named{method} {route}. Its attributes arehttp.request.method,http.route,url.scheme,network.protocol.version,http.response.status_codeand, for an error,error.type(the error envelope'scode).INTERNALchild span per decoded/classifyor/routerequest, with attributes prefixedgh_aw_router.:outcome;Metrics.
http.server.request.durationserror.typegh_aw_router.requests.rejectedgh_aw_router.reason:overloadorpayload_too_largegh_aw_router.planning.outcomesEvery metric attribute comes from a fixed set. Metrics never carry model ids or classifier labels.
Logs.
requestrecords gettrace_idandspan_idwhen the router started a span. With tracing off, they get only thetrace_idof a valid incomingtraceparent.telemetry configured,telemetry setting invalid,telemetry errorandtelemetry flush incomplete.tracestate.Decisions for maintainers
golang.org/x/net/http2, even though they only export over HTTP. They make up most of the size increase below.govulncheckin CI and the Grype gate keep these dependencies current.golang.org/x/netv0.60.0. With v0.59.0, which the exporters require,govulncheckreports five reachable HTTP/2 advisories: GO-2026-6603, -6610, -6611, -6612 and -6617. v0.60.0 fixes them.grpcandhttp/jsonlog a warning and turn the signal off.gh_aw_router., like the span attributes, so they can't collide with semconv names. A bare 503 has no error envelope, so it's recorded witherror.type503.tracestatestays on the router's spans, as W3C propagation requires, but is never logged.baggageis ignored.service.instance.idis a random UUID for each process. Metrics are cumulative, so without it two routers exporting to one collector would look like one counter resetting. The host, process and container detectors stay off.http.server.active_requests. It's a Development-status semconv instrument, and a router that serves a few requests per run gets nothing from a concurrency gauge.gh_aw_router.requests.rejectedrepeats what the histogram records for those requests, so that load shedding has one name to find and alert on.Validation
Go checks.
gofmt -l,go vetandgo test ./...pass incontainers/routeringolang:1.27.3-alpine.go test -race ./...passes ingolang:1.27.3-bookworm.Shutdown timing.
Vulnerability scans.
govulncheckv1.9.0 reports no vulnerabilities.supply-chain-scan.ymlpins) with the repo's.grype.yamland--fail-on highfinds none in the image.Container tests. The image builds from this branch, and the container tests pass against it, including the routing-contract corpus.
Licenses. All 24 modules linked into the binary are Apache-2.0, MIT or BSD-3-Clause.
Size.
linux/amd64linux/arm64linux/arm64)linux/arm64)Not tested here. Export from a router running inside AWF. AWF doesn't pass telemetry settings to the router until step 7, which adds an integration test with a collector on
awf-routing. With telemetry off, the router behaves as in feat(router): ship the Go router image and remove the Python implementation #10359, andtest-model-routingruns on the merge commit.Rollback
Telemetry is off unless the
OTEL_*variables are set, and AWF doesn't set them. Reverting this PR restores #10359's router with no other changes.