feat: add observability on subgraph connections and resolvers - #2881
Merged
alepane21 merged 52 commits intoJul 3, 2026
Merged
Conversation
…up-timeout-impacting-wundergraph-router-latency
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds resolver concurrency metrics and enhanced HTTP client-phase metrics (DNS, TCP, TLS, TTFB). Introduces config flags and schema updates, captures per-phase timings via httptrace, registers instruments in OTLP/Prometheus/no-op providers, wires MetricStore into GraphQL handler/websocket flows, registers executor resolvers with EngineStats, emits phase and fetch-processing spans, and updates tests to validate metrics and spans. ChangesResolver Concurrency and Enhanced Connection Metrics
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2881 +/- ##
==========================================
- Coverage 61.45% 61.42% -0.04%
==========================================
Files 260 261 +1
Lines 30282 30563 +281
==========================================
+ Hits 18609 18772 +163
- Misses 10176 10280 +104
- Partials 1497 1511 +14
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/core/graph_server.go`:
- Around line 1463-1467: The resolver is being registered with s.engineStats
before buildGraphMux has finished and error paths can leave it registered; move
the s.engineStats.RegisterResolver(executor.Resolver) call (and its
corresponding context.AfterFunc/unregister logic) so it runs only after
buildGraphMux completes without error, or alternatively add a cleanup path that
calls s.engineStats.UnregisterResolver(executor.Resolver) on any early returns;
update the code around buildGraphMux/graphMuxCtx/context.AfterFunc to ensure
executor.Resolver is only registered when the mux cannot fail and that
UnregisterResolver is always invoked on error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 311e666b-9a5a-40d5-ab26-6246279c9b08
📒 Files selected for processing (27)
router-tests/observability/prometheus_test.gorouter-tests/testenv/sync_reporter.gorouter-tests/testenv/testenv.gorouter/core/graph_server.gorouter/core/graphql_handler.gorouter/core/router.gorouter/core/trace_dialer.gorouter/core/websocket.gorouter/internal/traceclient/traceclient.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/metric/config.gorouter/pkg/metric/connection_measurements.gorouter/pkg/metric/connection_metric_store.gorouter/pkg/metric/engine_metrics.gorouter/pkg/metric/measurements.gorouter/pkg/metric/metric_store.gorouter/pkg/metric/noop_connection_metrics.gorouter/pkg/metric/noop_metrics.gorouter/pkg/metric/oltp_connection_metric_store.gorouter/pkg/metric/otlp_metric_store.gorouter/pkg/metric/prom_connection_metric_store.gorouter/pkg/metric/prom_metric_store.gorouter/pkg/statistics/engine_stats.go
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/core/transport.go (1)
202-257: ⚡ Quick winGuard the tracer provider before boxing it into the interface.
t.tracerProvideris a*sdktrace.TracerProvider, butNewCustomTransportnow takesotrace.TracerProvider. A nil pointer boxed into that interface is non-nil, so the downstreamopts.TracerProvider != nilcheck can succeed and callTracer(...)on a nil receiver.♻️ Suggested fix
func (t TransportFactory) RoundTripper(baseTransport http.RoundTripper) http.RoundTripper { if t.localhostFallbackInsideDocker && docker.Inside() { baseTransport = docker.NewLocalhostFallbackRoundTripper(baseTransport) } @@ traceTransport := trace.NewTransport( baseTransport, otelHttpOptions, trace.WithPreHandler(func(r *http.Request) { @@ }), ) + + var tracerProvider otrace.TracerProvider + if t.tracerProvider != nil { + tracerProvider = t.tracerProvider + } + tp := NewCustomTransport( traceTransport, t.retryOptions, t.metricStore, t.connectionMetricStore, t.circuitBreaker, t.enableTraceClient, - t.tracerProvider, + tracerProvider, t.emitConnectionPhaseSpan, )Also applies to: 307-315
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/core/transport.go` around lines 202 - 257, NewTransport is assigning opts.TracerProvider (a concrete *sdktrace.TracerProvider) directly into the TransportFactory.tracerProvider interface, which can box a nil pointer as non-nil; guard the assignment by checking if opts.TracerProvider != nil and only then set TransportFactory.tracerProvider = opts.TracerProvider, otherwise set it to a nil interface value (e.g. leave it nil or explicitly cast otrace.TracerProvider(nil)); apply the same nil-check pattern in the other constructor (NewCustomTransport / the block around lines 307-315) that assigns TracerProvider from TransportOptions so downstream calls to Tracer(...) never operate on a boxed nil receiver.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@router/core/transport.go`:
- Around line 202-257: NewTransport is assigning opts.TracerProvider (a concrete
*sdktrace.TracerProvider) directly into the TransportFactory.tracerProvider
interface, which can box a nil pointer as non-nil; guard the assignment by
checking if opts.TracerProvider != nil and only then set
TransportFactory.tracerProvider = opts.TracerProvider, otherwise set it to a nil
interface value (e.g. leave it nil or explicitly cast
otrace.TracerProvider(nil)); apply the same nil-check pattern in the other
constructor (NewCustomTransport / the block around lines 307-315) that assigns
TracerProvider from TransportOptions so downstream calls to Tracer(...) never
operate on a boxed nil receiver.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64974d72-3fda-4020-ace7-11b31f169b56
📒 Files selected for processing (14)
router-tests/telemetry/connection_phase_spans_test.gorouter-tests/testenv/testenv.gorouter/core/graph_server.gorouter/core/graphql_handler.gorouter/core/router.gorouter/core/transport.gorouter/core/websocket.gorouter/internal/traceclient/traceclient.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/trace/config.go
…up-timeout-impacting-wundergraph-router-latency
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router-tests/telemetry/connection_phase_spans_test.go (1)
61-133:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWait for the expected spans before asserting on them.
Both enabled-path subtests read
exporter.GetSpans().Snapshots()exactly once right after the request. That makes the assertions onResolver - Acquire, response-body, and response-processing spans timing-sensitive. Wrap the snapshot/assertion block inrequire.Eventuallyand wait until all expected span names are present before checking parentage and durations.As per coding guidelines: For periodic exporters, wait for ALL expected items using require.Eventually, not just one sentinel value, to avoid race conditions with export cycles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router-tests/telemetry/connection_phase_spans_test.go` around lines 61 - 133, The test reads exporter.GetSpans().Snapshots() once and asserts on timing-sensitive spans; wrap the snapshot + assertion logic in a require.Eventually to wait until all expected spans are present before checking parentage/durations. Specifically, in the blocks using exporter.GetSpans().Snapshots(), call require.Eventually and inside its closure repeatedly call exporter.GetSpans().Snapshots() and assert presence of names via hasSpanWithName / spansByName (e.g. "HTTP - TCP Connect", "HTTP - Time To First Byte", "HTTP - Read Response Body", "Engine - Fetch Response Processing", and "Resolver - Acquire"), then only after the Eventually returns proceed to use indexByID, spansByName and the duration/parent assertions; pick a reasonable timeout/tick (e.g. a few seconds) so the periodic exporter can flush.
♻️ Duplicate comments (1)
router/core/graph_server.go (1)
1465-1469:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegister the resolver only after the remaining setup can no longer fail.
buildGraphMuxstill has several error returns after Line 1465. On any of those paths,graphMuxCtxis never canceled, so thisAfterFuncnever runs and the resolver stays registered ins.engineStats, leaving stale resolver-concurrency metrics behind on failed reloads/tests. Move this block to the last no-fail section before success, or explicitly unregister on every later error path.Based on learnings: ensure that buildGraphMux error paths clean up partially initialized resources (caches, metric stores, pub/sub providers, connectors) before returning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/core/graph_server.go` around lines 1465 - 1469, The resolver is registered with s.engineStats (via s.engineStats.RegisterResolver(executor.Resolver)) before buildGraphMux finishes and later error returns can leave it registered; move the RegisterResolver + context.AfterFunc(UnregisterResolver) pair to the final no-fail section of buildGraphMux (after all remaining initialization succeeds) so registration happens only on success, or alternatively ensure every subsequent error return path explicitly calls s.engineStats.UnregisterResolver(executor.Resolver) (or cancels graphMuxCtx) to clean up; locate the current calls to s.engineStats.RegisterResolver and s.engineStats.UnregisterResolver and adjust them around buildGraphMux, graphMuxCtx, and executor.Resolver accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@router-tests/telemetry/connection_phase_spans_test.go`:
- Around line 61-133: The test reads exporter.GetSpans().Snapshots() once and
asserts on timing-sensitive spans; wrap the snapshot + assertion logic in a
require.Eventually to wait until all expected spans are present before checking
parentage/durations. Specifically, in the blocks using
exporter.GetSpans().Snapshots(), call require.Eventually and inside its closure
repeatedly call exporter.GetSpans().Snapshots() and assert presence of names via
hasSpanWithName / spansByName (e.g. "HTTP - TCP Connect", "HTTP - Time To First
Byte", "HTTP - Read Response Body", "Engine - Fetch Response Processing", and
"Resolver - Acquire"), then only after the Eventually returns proceed to use
indexByID, spansByName and the duration/parent assertions; pick a reasonable
timeout/tick (e.g. a few seconds) so the periodic exporter can flush.
---
Duplicate comments:
In `@router/core/graph_server.go`:
- Around line 1465-1469: The resolver is registered with s.engineStats (via
s.engineStats.RegisterResolver(executor.Resolver)) before buildGraphMux finishes
and later error returns can leave it registered; move the RegisterResolver +
context.AfterFunc(UnregisterResolver) pair to the final no-fail section of
buildGraphMux (after all remaining initialization succeeds) so registration
happens only on success, or alternatively ensure every subsequent error return
path explicitly calls s.engineStats.UnregisterResolver(executor.Resolver) (or
cancels graphMuxCtx) to clean up; locate the current calls to
s.engineStats.RegisterResolver and s.engineStats.UnregisterResolver and adjust
them around buildGraphMux, graphMuxCtx, and executor.Resolver accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18be247e-ae39-4b8d-94f4-c632d5f4d407
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
router-tests/go.modrouter-tests/telemetry/connection_phase_spans_test.gorouter/core/engine_loader_hooks.gorouter/core/engine_loader_hooks_test.gorouter/core/graph_server.gorouter/core/graphql_handler.gorouter/core/transport.gorouter/core/websocket.gorouter/go.modrouter/internal/context/fetch_trace_timings.gorouter/internal/context/keys.gorouter/pkg/trace/transport.gorouter/pkg/trace/transport_test.go
✅ Files skipped from review due to trivial changes (1)
- router/internal/context/fetch_trace_timings.go
…up-timeout-impacting-wundergraph-router-latency
…up-timeout-impacting-wundergraph-router-latency
…ndergraph-router-latency
…impacting-wundergraph-router-latency' into ale/eng-9608-dns-lookup-timeout-impacting-wundergraph-router-latency
…ndergraph-router-latency
…up-timeout-impacting-wundergraph-router-latency
alepane21
marked this pull request as ready for review
June 5, 2026 09:05
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
…up-timeout-impacting-wundergraph-router-latency
…up-timeout-impacting-wundergraph-router-latency
…up-timeout-impacting-wundergraph-router-latency
dkorittki
reviewed
Jul 1, 2026
dkorittki
left a comment
dkorittki
left a comment
Contributor
There was a problem hiding this comment.
lgtm besides what I commented
dkorittki
approved these changes
Jul 1, 2026
…ndergraph-router-latency
…up-timeout-impacting-wundergraph-router-latency
…ndergraph-router-latency
…ndergraph-router-latency
…ndergraph-router-latency
Noroth
approved these changes
Jul 3, 2026
alepane21
deleted the
ale/eng-9608-dns-lookup-timeout-impacting-wundergraph-router-latency
branch
July 3, 2026 10:43
6 tasks
6 tasks
This was referenced Jul 27, 2026
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
3 participants
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.
This PR add additional metrics on connection acquisition:
Also expose resolvers usage:
And finally operation variables:
This new variables are also available in the access logs or subgraph access logs, depending on where it makes sense. On the subgraph access logs we also added access to subgraph response headers.
Summary by CodeRabbit
New Features
Configuration
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.