feat(router): engine flags for dataflow execution and execution plan scheduling#2961
Draft
jensneuse wants to merge 6 commits into
Draft
feat(router): engine flags for dataflow execution and execution plan scheduling#2961jensneuse wants to merge 6 commits into
jensneuse wants to merge 6 commits into
Conversation
Contributor
WalkthroughAdds two engine execution flags (dataflow, execution-plan scheduling), wires them through planner/executor/graph-server and tests, reduces per-fetch telemetry when disabled, adds a latency-injection proxy, and adds byte-identity, uniform, and skew benchmark scripts and documentation. ChangesExecution Modes and Benchmarking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![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: 2
🧹 Nitpick comments (6)
router-tests/bench/engine-modes/README.md (1)
25-27: ⚡ Quick winClarify working directory assumptions in command blocks.
Line 25 changes cwd to
router, while Lines 42 and 57-62 use relative paths/scripts that appear to assumerouter-tests/bench/engine-modes. Make cwd explicit so copy/paste execution is reproducible.Proposed command hardening
-cd router && CGO_ENABLED=0 GOOS=linux GOARCH=$(go env GOARCH) go build -ldflags '-s -w' -o /tmp/bench-router/cosmo ./cmd/router +(cd router && CGO_ENABLED=0 GOOS=linux GOARCH=$(go env GOARCH) go build -ldflags '-s -w' -o /tmp/bench-router/cosmo ./cmd/router) -docker build -t lat-proxy:local lat-proxy/ +docker build -t lat-proxy:local router-tests/bench/engine-modes/lat-proxy/ -bash byteid.sh -RES=/tmp/results/uniform.jsonl bash uniform_matrix.sh -COSMO_CFG_DIR=/path/to/cfg-proxy RES=/tmp/results/skew.jsonl bash skew_matrix.sh +(cd router-tests/bench/engine-modes && bash byteid.sh) +(cd router-tests/bench/engine-modes && RES=/tmp/results/uniform.jsonl bash uniform_matrix.sh) +(cd router-tests/bench/engine-modes && COSMO_CFG_DIR=/path/to/cfg-proxy RES=/tmp/results/skew.jsonl bash skew_matrix.sh)Also applies to: 42-43, 57-62
🤖 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/bench/engine-modes/README.md` around lines 25 - 27, The README command blocks assume different current directories (e.g., the `cd router` at the build step vs later relative paths used in other blocks); make the working directory explicit and robust by either (A) prepping each block with an absolute or repo-root-relative `cd` (or `pushd`/`popd`) to the intended directory before running the commands, or (B) replace relative paths with explicit paths (e.g., use `./router/...` or `/tmp/bench-router/...`) so the `go build` (`-o /tmp/bench-router/cosmo`), `printf`/Dockerfile creation, and `docker build -t bench-cosmo:local /tmp/bench-router` commands work when copy/pasted; update the blocks that start with `cd router` and the later blocks at lines ~42 and ~57-62 to follow one consistent approach.router-tests/bench/engine-modes/uniform_bench.sh (1)
101-101: 💤 Low valueConsider adding
-rflag toreadcommand.ShellCheck warns that
readwithout-rwill mangle backslashes. While docker stats output is unlikely to contain backslashes, adding-rwould be more defensive.♻️ Proposed fix
-read CPUAVG CPUMAX MEMMAX <<<"$(tr -d '\r' < "$STATF" | sed 's/\x1b\[[0-9;]*[a-zA-Z]//g' | awk -F'|' '{gsub(/%/,"",$1);c=$1+0;if(c>cm)cm=c;cs+=c;n++;split($2,a,"/");m=a[1];gsub(/[^0-9.A-Za-z]/,"",m);v=m+0;if(m~/GiB/)v*=1024;if(m~/KiB/)v/=1024;if(v>mm)mm=v} END{if(n)printf "%.0f %.0f %.0f",cs/n,cm,mm;else print "0 0 0"}')" +read -r CPUAVG CPUMAX MEMMAX <<<"$(tr -d '\r' < "$STATF" | sed 's/\x1b\[[0-9;]*[a-zA-Z]//g' | awk -F'|' '{gsub(/%/,"",$1);c=$1+0;if(c>cm)cm=c;cs+=c;n++;split($2,a,"/");m=a[1];gsub(/[^0-9.A-Za-z]/,"",m);v=m+0;if(m~/GiB/)v*=1024;if(m~/KiB/)v/=1024;if(v>mm)mm=v} END{if(n)printf "%.0f %.0f %.0f",cs/n,cm,mm;else print "0 0 0"}')"🤖 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/bench/engine-modes/uniform_bench.sh` at line 101, The read invocation that parses CPUAVG/CPUMAX/MEMMAX (the line starting with read CPUAVG CPUMAX MEMMAX <<<"$(tr -d '\r' < "$STATF" | ... )") should use the -r flag to prevent backslash interpretation; change it to use read -r CPUAVG CPUMAX MEMMAX so backslashes are not treated as escape characters and the parsed fields are preserved.router-tests/bench/engine-modes/skew_bench.sh (1)
69-69: 💤 Low valueConsider adding
-rflag toreadcommand.ShellCheck warns that
readwithout-rwill mangle backslashes. While docker stats output is unlikely to contain backslashes, adding-rwould be more defensive. (Same issue as inuniform_bench.sh.)♻️ Proposed fix
-read CPUAVG CPUMAX MEMMAX <<<"$(tr -d '\r' < "$STATF" | sed 's/\x1b\[[0-9;]*[a-zA-Z]//g' | awk -F'|' '{gsub(/%/,"",$1);c=$1+0;if(c>cm)cm=c;cs+=c;n++;split($2,a,"/");m=a[1];gsub(/[^0-9.A-Za-z]/,"",m);v=m+0;if(m~/GiB/)v*=1024;if(m~/KiB/)v/=1024;if(v>mm)mm=v} END{if(n)printf "%.0f %.0f %.0f",cs/n,cm,mm;else print "0 0 0"}')" +read -r CPUAVG CPUMAX MEMMAX <<<"$(tr -d '\r' < "$STATF" | sed 's/\x1b\[[0-9;]*[a-zA-Z]//g' | awk -F'|' '{gsub(/%/,"",$1);c=$1+0;if(c>cm)cm=c;cs+=c;n++;split($2,a,"/");m=a[1];gsub(/[^0-9.A-Za-z]/,"",m);v=m+0;if(m~/GiB/)v*=1024;if(m~/KiB/)v/=1024;if(v>mm)mm=v} END{if(n)printf "%.0f %.0f %.0f",cs/n,cm,mm;else print "0 0 0"}')"🤖 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/bench/engine-modes/skew_bench.sh` at line 69, The read invocation that parses CPUAVG CPUMAX MEMMAX from the subshell should use the -r flag to avoid backslash interpretation; update the command that currently reads into CPUAVG CPUMAX MEMMAX (the read ... <<<"$(...)" line that consumes $STATF output) to use read -r so backslashes are not mangled.router-tests/bench/engine-modes/byteid.sh (1)
80-88: 💤 Low valueConsider using a glob count instead of
ls | wc -l.ShellCheck flags the
ls *.json | wc -lpattern (line 83) as potentially problematic with non-alphanumeric filenames. While this works fine in the controlled benchmark context, a direct glob expansion would be more robust.♻️ Alternative implementation using glob
local hashes bytes nfiles hashes=$(shasum -a 256 "$outdir"/*.json | awk '{print $1}' | sort -u) bytes=$(wc -c < "$outdir/seq_1.json" | tr -d ' ') - nfiles=$(ls "$outdir"/*.json | wc -l | tr -d ' ') + nfiles=$(set -- "$outdir"/*.json; echo "$#") # The gate is 25 sequential + 25 concurrent; a partial concurrent batch🤖 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/bench/engine-modes/byteid.sh` around lines 80 - 88, Replace the fragile "ls \"$outdir\"/*.json | wc -l" count with a proper glob-based count: use a shell array to expand "$outdir"/*.json into files (e.g., files=( "$outdir"/*.json )) and then set nfiles=${`#files`[@]}; keep the existing variables (hashes, bytes, nfiles) and the same failure check, and ensure this change is made where nfiles is assigned in the BYTEID check block so non-alphanumeric filenames are handled safely.router-tests/bench/engine-modes/lat-proxy/Dockerfile (1)
6-8: 💤 Low valueConsider adding a non-root user for security.
The container runs as root, which violates the principle of least privilege. While this may be acceptable for a benchmark tool used in controlled environments, adding a non-root user would improve the security posture.
🔒 Recommended fix to add non-root user
FROM alpine:3.20 +RUN addgroup -S latproxy && adduser -S latproxy -G latproxy COPY --from=build /lat-proxy /lat-proxy +RUN chown latproxy:latproxy /lat-proxy +USER latproxy ENTRYPOINT ["/lat-proxy"]🤖 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/bench/engine-modes/lat-proxy/Dockerfile` around lines 6 - 8, The Dockerfile runs the binary /lat-proxy as root; add a non-root user and switch to it before ENTRYPOINT to follow least-privilege. Create a user (e.g., latuser), create a group if desired, set ownership of /lat-proxy to that user (chown), ensure any needed directories are writable by that user, and add a USER latuser line before ENTRYPOINT so the container runs the lat-proxy process unprivileged.router/core/engine_loader_hooks_test.go (1)
50-131: ⚡ Quick winAdd one regression case for tracing-disabled hook behavior.
All updated calls use
true, true; please add a subtest withtracingEnabled=false(e.g., metrics-only) to assert fetch hooks do not end/mutate parent spans.🤖 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/engine_loader_hooks_test.go` around lines 50 - 131, Add a new subtest "tracing disabled does not end/mutate parent span" that creates hooks via NewEngineRequestHooks with tracingEnabled=false (use same arg pattern as other tests), uses setupTestContext to create a parent span, calls hooks.OnFinished(...) (similar to the other cases), and then asserts the hooks did not end or change the parent span (e.g., exporter.GetSpans().Snapshots() should show the parent span unchanged: not marked Error, not ended/mutated, and no extra span events created by the hook); also verify metric behavior remains as expected via spyMetricStore.requestErrorCalled where applicable.
🤖 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-tests/bench/engine-modes/byteid.sh`:
- Around line 73-76: The helper script generation writes unquoted variables into
"$WORK/byteid_curl.sh" which can cause word splitting; change the heredoc to a
literal (use <<'HELPER' instead of <<HELPER) and quote the runtime variables
inside the docker command (e.g., --network "$NET", -v "$QUERY_FILE":/q.json:ro,
-X POST "$EP", and redirect to "$outdir/conc_$1.json") so the generated script
is robust against spaces/special chars while preserving the literal $1 in the
created script.
In `@router/core/engine_loader_hooks.go`:
- Around line 122-129: OnLoad currently creates the "Engine - Fetch" span only
when f.tracingEnabled, but OnFinished unconditionally calls
trace.SpanFromContext(ctx) and mutates/ends the span; fix by guarding all span
lifecycle and mutation code behind the same f.tracingEnabled check (or verify
the span is the one we created via Span.SpanContext().IsValid()) so we do not
end or record errors on a parent span when tracing is disabled. Update OnLoad
and OnFinished in router/core/engine_loader_hooks.go (the f.tracingEnabled block
in OnLoad and all span uses in OnFinished) to either only obtain/operate on
trace.SpanFromContext(ctx) when f.tracingEnabled is true or to check the span
validity before calling span.End(), span.RecordError(), recordFetchError(...,
span, ...), and span.SetAttributes(...).
---
Nitpick comments:
In `@router-tests/bench/engine-modes/byteid.sh`:
- Around line 80-88: Replace the fragile "ls \"$outdir\"/*.json | wc -l" count
with a proper glob-based count: use a shell array to expand "$outdir"/*.json
into files (e.g., files=( "$outdir"/*.json )) and then set nfiles=${`#files`[@]};
keep the existing variables (hashes, bytes, nfiles) and the same failure check,
and ensure this change is made where nfiles is assigned in the BYTEID check
block so non-alphanumeric filenames are handled safely.
In `@router-tests/bench/engine-modes/lat-proxy/Dockerfile`:
- Around line 6-8: The Dockerfile runs the binary /lat-proxy as root; add a
non-root user and switch to it before ENTRYPOINT to follow least-privilege.
Create a user (e.g., latuser), create a group if desired, set ownership of
/lat-proxy to that user (chown), ensure any needed directories are writable by
that user, and add a USER latuser line before ENTRYPOINT so the container runs
the lat-proxy process unprivileged.
In `@router-tests/bench/engine-modes/README.md`:
- Around line 25-27: The README command blocks assume different current
directories (e.g., the `cd router` at the build step vs later relative paths
used in other blocks); make the working directory explicit and robust by either
(A) prepping each block with an absolute or repo-root-relative `cd` (or
`pushd`/`popd`) to the intended directory before running the commands, or (B)
replace relative paths with explicit paths (e.g., use `./router/...` or
`/tmp/bench-router/...`) so the `go build` (`-o /tmp/bench-router/cosmo`),
`printf`/Dockerfile creation, and `docker build -t bench-cosmo:local
/tmp/bench-router` commands work when copy/pasted; update the blocks that start
with `cd router` and the later blocks at lines ~42 and ~57-62 to follow one
consistent approach.
In `@router-tests/bench/engine-modes/skew_bench.sh`:
- Line 69: The read invocation that parses CPUAVG CPUMAX MEMMAX from the
subshell should use the -r flag to avoid backslash interpretation; update the
command that currently reads into CPUAVG CPUMAX MEMMAX (the read ... <<<"$(...)"
line that consumes $STATF output) to use read -r so backslashes are not mangled.
In `@router-tests/bench/engine-modes/uniform_bench.sh`:
- Line 101: The read invocation that parses CPUAVG/CPUMAX/MEMMAX (the line
starting with read CPUAVG CPUMAX MEMMAX <<<"$(tr -d '\r' < "$STATF" | ... )")
should use the -r flag to prevent backslash interpretation; change it to use
read -r CPUAVG CPUMAX MEMMAX so backslashes are not treated as escape characters
and the parsed fields are preserved.
In `@router/core/engine_loader_hooks_test.go`:
- Around line 50-131: Add a new subtest "tracing disabled does not end/mutate
parent span" that creates hooks via NewEngineRequestHooks with
tracingEnabled=false (use same arg pattern as other tests), uses
setupTestContext to create a parent span, calls hooks.OnFinished(...) (similar
to the other cases), and then asserts the hooks did not end or change the parent
span (e.g., exporter.GetSpans().Snapshots() should show the parent span
unchanged: not marked Error, not ended/mutated, and no extra span events created
by the hook); also verify metric behavior remains as expected via
spyMetricStore.requestErrorCalled where applicable.
🪄 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: cc458c99-23a4-4d0d-a37d-e53cde07e88a
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
router-tests/bench/engine-modes/README.mdrouter-tests/bench/engine-modes/byteid.shrouter-tests/bench/engine-modes/k6.jsrouter-tests/bench/engine-modes/lat-proxy/Dockerfilerouter-tests/bench/engine-modes/lat-proxy/main.gorouter-tests/bench/engine-modes/skew_bench.shrouter-tests/bench/engine-modes/skew_matrix.shrouter-tests/bench/engine-modes/uniform_bench.shrouter-tests/bench/engine-modes/uniform_matrix.shrouter-tests/go.modrouter-tests/operations/execution_plan_scheduling_test.gorouter-tests/testenv/testenv.gorouter/core/context.gorouter/core/engine_loader_hooks.gorouter/core/engine_loader_hooks_test.gorouter/core/executor.gorouter/core/graph_server.gorouter/core/operation_planner.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/config_test.gorouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
Comment on lines
+73
to
+76
| cat > "$WORK/byteid_curl.sh" <<HELPER | ||
| #!/bin/sh | ||
| docker run --rm --network $NET -v $QUERY_FILE:/q.json:ro curlimages/curl:latest -fsS --max-time 30 -X POST $EP -H 'content-type: application/json' --data-binary @/q.json > $outdir/conc_\$1.json 2>/dev/null | ||
| HELPER |
Contributor
There was a problem hiding this comment.
Quote variables in the generated helper script to prevent word splitting.
The generated helper script at line 75 contains unquoted shell variables ($NET, $QUERY_FILE, $EP, $outdir) that could cause word splitting or globbing if any of these values contain spaces or special characters.
While the current usage context (controlled benchmark environment) makes this unlikely, quoting the variables would be more robust.
🛡️ Proposed fix to quote variables
cat > "$WORK/byteid_curl.sh" <<HELPER
#!/bin/sh
-docker run --rm --network $NET -v $QUERY_FILE:/q.json:ro curlimages/curl:latest -fsS --max-time 30 -X POST $EP -H 'content-type: application/json' --data-binary `@/q.json` > $outdir/conc_\$1.json 2>/dev/null
+docker run --rm --network "$NET" -v "$QUERY_FILE":/q.json:ro curlimages/curl:latest -fsS --max-time 30 -X POST "$EP" -H 'content-type: application/json' --data-binary `@/q.json` > "$outdir"/conc_\$1.json 2>/dev/null
HELPER📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cat > "$WORK/byteid_curl.sh" <<HELPER | |
| #!/bin/sh | |
| docker run --rm --network $NET -v $QUERY_FILE:/q.json:ro curlimages/curl:latest -fsS --max-time 30 -X POST $EP -H 'content-type: application/json' --data-binary @/q.json > $outdir/conc_\$1.json 2>/dev/null | |
| HELPER | |
| cat > "$WORK/byteid_curl.sh" <<HELPER | |
| #!/bin/sh | |
| docker run --rm --network "$NET" -v "$QUERY_FILE":/q.json:ro curlimages/curl:latest -fsS --max-time 30 -X POST "$EP" -H 'content-type: application/json' --data-binary `@/q.json` > "$outdir"/conc_\$1.json 2>/dev/null | |
| HELPER |
🤖 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/bench/engine-modes/byteid.sh` around lines 73 - 76, The helper
script generation writes unquoted variables into "$WORK/byteid_curl.sh" which
can cause word splitting; change the heredoc to a literal (use <<'HELPER'
instead of <<HELPER) and quote the runtime variables inside the docker command
(e.g., --network "$NET", -v "$QUERY_FILE":/q.json:ro, -X POST "$EP", and
redirect to "$outdir/conc_$1.json") so the generated script is robust against
spaces/special chars while preserving the literal $1 in the created script.
…scheduling Adds EngineExecutionConfiguration.EnableDataflowExecution (ENGINE_ENABLE_DATAFLOW) and EnableExecutionPlanScheduling (ENGINE_ENABLE_SCHEDULE_TREE), both default off. The first enables the engine's per-FetchID dataflow executor for flat query plans; the second plans operations through the dominance-based schedule-tree scheduler (constructor-injected into OperationPlanner — no env reads in the planner). Schedule-tree plans are nested, and the dataflow executor structurally rejects nested plans and falls back to the wave executor, so the combination degrades safely (warn-logged at startup). TestExecutionPlanSchedulingPlumbing pins the full response of a multi-subgraph operation under the scheduler, proving config -> engine plumbing and the nested Parallel(Sequence...) plan shape end to end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…st context keys Engine loader hooks (OnLoad/OnFinished) skip span creation, attribute building, and metric measurement when tracing, metrics, and subgraph access logging are all disabled — header propagation still runs. The default tracer was an SDK NeverSample provider that still allocated a span per call. requestContext.keys is now lazily allocated on first Set instead of eagerly per request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… in testenv Initializes EnableDataflowExecution / EnableExecutionPlanScheduling from ENGINE_ENABLE_DATAFLOW / ENGINE_ENABLE_SCHEDULE_TREE so the whole suite re-runs per engine mode without code changes (test harness only — production config is constructor-injected). ModifyEngineExecutionConfiguration still overrides per test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Byte-identity gate (one sha256 across baseline/dataflow/scheduler/both, 25 sequential + 25 concurrent requests per mode, env->config->engine plumb-through proven via log greps) plus uniform-netem and skewed-latency matrices for the three engine modes from ONE router image. Encodes the methodology that made the reference numbers trustworthy: netem limit 100000 with qdisc verification, CPU-pinned gateway, real-response readiness gates, valid_bytes per row, warmup-capped k6, interleaved median-of-3. Includes the per-path lat-proxy source. Cosmo-only by design — no competitor images or configs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…workload attribution (codex P2s) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pins both router modules to github.com/wundergraph/graphql-go-tools/v2@v2.4.6-0.20260611164326-f75f280f48dd (wundergraph/graphql-go-tools#1535: schedule-tree scheduler + dataflow executor, rebased on v2.4.5/go-arena v1.3.0). To be moved to a tagged release once that PR merges. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jensneuse
force-pushed
the
feat/engine-schedule-tree-dataflow
branch
from
3e3a363 to
f4b600a
Compare
❌ Internal Query Planner CI checks failedThe Internal Query Planner CI checks failed in the celestial repository, and this is going to stop the merge of this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2961 +/- ##
=======================================
Coverage 66.36% 66.37%
=======================================
Files 258 258
Lines 27539 27558 +19
=======================================
+ Hits 18277 18291 +14
- Misses 7812 7816 +4
- Partials 1450 1451 +1
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/core/engine_loader_hooks.go (1)
177-178:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard all span operations behind
tracingEnabledcheck to prevent corrupting parent span lifecycle.The early-return fast path (lines 168-173) only covers the case where tracing, metrics, and logging are all disabled. When
tracingEnabled=falsebutmetricsEnabled=true(oraccessLogger != nil), execution continues past the early return and unconditionally:
- Line 177:
span := trace.SpanFromContext(ctx)retrieves a span from context (could be a parent span from HTTP middleware, since OnLoad didn't create one whentracingEnabled=false)- Line 178:
defer span.End()ends that span- Lines 274, 280, 287: Call
span.RecordError(),recordFetchError(..., span, ...), andspan.SetAttributes()on that spanThis violates span lifecycle management: we're ending and mutating a span we didn't create. If a parent span exists in the context, this will prematurely end it and corrupt the tracing hierarchy.
Guard all span retrieval and operations behind
if f.tracingEnabled { ... }checks.🔒 Proposed fix to guard span lifecycle
- span := trace.SpanFromContext(ctx) - defer span.End() + var span trace.Span + if f.tracingEnabled { + span = trace.SpanFromContext(ctx) + defer span.End() + } commonAttrs := []attribute.KeyValue{ semconv.HTTPStatusCode(responseInfo.StatusCode), rotel.WgSubgraphID.String(ds.ID), rotel.WgSubgraphName.String(ds.Name), } traceAttrs := *reqContext.telemetry.AcquireAttributes() defer reqContext.telemetry.ReleaseAttributes(&traceAttrs) traceAttrs = append(traceAttrs, reqContext.telemetry.traceAttrs...) traceAttrs = append(traceAttrs, rotel.WgComponentName.String("engine-loader")) traceAttrs = append(traceAttrs, commonAttrs...) exprCtx := reqContext.expressionContext.Clone() exprCtx.Subgraph.Id = ds.ID exprCtx.Subgraph.Name = ds.Name exprCtx.Subgraph.Request.Error = WrapExprError(responseInfo.Err) if value := ctx.Value(rcontext.FetchTimingKey); value != nil { if fetchTiming, ok := value.(*atomic.Int64); ok { exprCtx.Subgraph.Request.ClientTrace.FetchDuration = time.Duration(fetchTiming.Load()) } } if f.storeSubgraphResponseBody { exprCtx.Subgraph.Response.Body.Raw = responseInfo.GetResponseBody() } metricAttrs := *reqContext.telemetry.AcquireAttributes() defer reqContext.telemetry.ReleaseAttributes(&metricAttrs) metricAttrs = append(metricAttrs, reqContext.telemetry.metricAttrs...) metricAttrs = append(metricAttrs, commonAttrs...) - addExpressions(AddExprOpts{ - logger: reqContext.logger, - expressions: f.telemetryAttributeExpressions, - key: expr.BucketSubgraph, - currSpan: span, - exprCtx: exprCtx, - attrAddFunc: func(telemetryValues ...attribute.KeyValue) { - traceAttrs = append(traceAttrs, telemetryValues...) - metricAttrs = append(metricAttrs, telemetryValues...) - }, - }) - addExpressions(AddExprOpts{ - logger: reqContext.logger, - expressions: f.tracingAttributeExpressions, - key: expr.BucketSubgraph, - currSpan: span, - exprCtx: exprCtx, - attrAddFunc: func(telemetryValues ...attribute.KeyValue) { - traceAttrs = append(traceAttrs, telemetryValues...) - }, - }) + if f.tracingEnabled { + addExpressions(AddExprOpts{ + logger: reqContext.logger, + expressions: f.telemetryAttributeExpressions, + key: expr.BucketSubgraph, + currSpan: span, + exprCtx: exprCtx, + attrAddFunc: func(telemetryValues ...attribute.KeyValue) { + traceAttrs = append(traceAttrs, telemetryValues...) + metricAttrs = append(metricAttrs, telemetryValues...) + }, + }) + addExpressions(AddExprOpts{ + logger: reqContext.logger, + expressions: f.tracingAttributeExpressions, + key: expr.BucketSubgraph, + currSpan: span, + exprCtx: exprCtx, + attrAddFunc: func(telemetryValues ...attribute.KeyValue) { + traceAttrs = append(traceAttrs, telemetryValues...) + }, + }) + } addExpressions(AddExprOpts{ logger: reqContext.logger, expressions: f.metricAttributeExpressions, key: expr.BucketSubgraph, exprCtx: exprCtx, attrAddFunc: func(telemetryValues ...attribute.KeyValue) { metricAttrs = append(metricAttrs, telemetryValues...) }, }) metricAddOpt := otelmetric.WithAttributeSet(attribute.NewSet(metricAttrs...)) // ... (access logger block unchanged) measureSliceAttrs := reqContext.telemetry.metricSliceAttrs if responseInfo.Err != nil { // Client disconnections (context.Canceled) are not server-side errors. // Record the error for observability but don't set the span status to ERROR // and don't count it as a request error in metrics. if errors.Is(responseInfo.Err, context.Canceled) { - span.RecordError(responseInfo.Err) + if f.tracingEnabled { + span.RecordError(responseInfo.Err) + } } else { errorSliceAttrs := *reqContext.telemetry.AcquireAttributes() defer reqContext.telemetry.ReleaseAttributes(&errorSliceAttrs) errorSliceAttrs = append(errorSliceAttrs, reqContext.telemetry.metricSliceAttrs...) - measureSliceAttrs, metricAddOpt = f.recordFetchError(ctx, span, responseInfo.Err, reqContext, metricAttrs, metricAddOpt, errorSliceAttrs) + if f.tracingEnabled { + measureSliceAttrs, metricAddOpt = f.recordFetchError(ctx, span, responseInfo.Err, reqContext, metricAttrs, metricAddOpt, errorSliceAttrs) + } else { + // Metrics-only path: still need to record the error metric + f.metricStore.MeasureRequestError(ctx, errorSliceAttrs, metricAddOpt) + } } } f.metricStore.MeasureRequestCount(ctx, measureSliceAttrs, metricAddOpt) f.metricStore.MeasureLatency(ctx, latency, measureSliceAttrs, metricAddOpt) - span.SetAttributes(traceAttrs...) + if f.tracingEnabled { + span.SetAttributes(traceAttrs...) + }Also applies to: 274-274, 280-280, 287-287
🤖 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/engine_loader_hooks.go` around lines 177 - 178, The span is retrieved and ended unconditionally which can prematurely close a parent span; wrap the span lifecycle and all span operations inside an if f.tracingEnabled { ... } block: move the span := trace.SpanFromContext(ctx) and defer span.End() into that guard, and only call span.RecordError(...), recordFetchError(..., span, ...), span.SetAttributes(...) when f.tracingEnabled is true (or pass nil/skip calls otherwise). Update all usages referenced (span variable, span.RecordError, recordFetchError invocations that accept span, and span.SetAttributes) so they execute only when f.tracingEnabled to avoid mutating/ending spans the code didn’t create.
🧹 Nitpick comments (2)
router-tests/bench/engine-modes/lat-proxy/Dockerfile (2)
1-8: 💤 Low valueContainer runs as root.
The static analysis tool correctly identifies that no USER instruction is present, so the container runs as root. While this is a security concern for production containers, it's generally acceptable for short-lived benchmark utilities in controlled environments.
If you want to harden this for broader use, consider adding a non-root user:
🔒 Optional: Add non-root user
FROM alpine:3.20 +RUN addgroup -g 10001 appuser && adduser -D -u 10001 -G appuser appuser COPY --from=build /lat-proxy /lat-proxy +USER appuser ENTRYPOINT ["/lat-proxy"]🤖 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/bench/engine-modes/lat-proxy/Dockerfile` around lines 1 - 8, The Dockerfile currently leaves the final image running as root because there is no USER instruction; to fix, in the final stage add creation of a non-root user (e.g., adduser or addgroup + useradd) and set ownership of the installed binary (/lat-proxy) to that user, then add a USER <username> line before ENTRYPOINT so the container runs as the non-root user; update the final stage that copies /lat-proxy to perform chown and create the user so the binary is executable by that user.Source: Linters/SAST tools
1-1: Go 1.25-alpine tag is present; root-user is only because no USER is set.
FROM golang:1.25-alpine(line 1) uses a tag that exists, so this Docker build shouldn’t fail due to the Go base image tag.- No
USERdirective is set, so the final image runs as root by default (minor policy consideration).🤖 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/bench/engine-modes/lat-proxy/Dockerfile` at line 1, The Dockerfile currently uses "FROM golang:1.25-alpine AS build" and leaves out a USER directive so the container runs as root; update the Dockerfile to create a non-root user (e.g., add a group and user with addgroup/adduser or adduser -D) during the build stage, chown any application directories to that user, and add a "USER <username>" directive in the final stage so the container does not run as root; ensure the steps reference the existing "FROM golang:1.25-alpine AS build" stage and the new "USER" directive to switch context.
🤖 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-tests/operations/execution_plan_scheduling_test.go`:
- Line 40: The test currently does a brittle string equality on the entire
response body (require.Equal comparing the raw JSON in res.Body), which can
flake due to non-deterministic ordering in nested plan children; change this to
use structural JSON comparison and targeted assertions: replace the raw
require.Equal with require.JSONEq(expectedJSON, string(res.Body)) for structural
equality, then unmarshal res.Body into a map/struct and assert only the
essential schedule-tree invariants (e.g., presence of "Parallel"/"Sequence"
nodes and specific fetch entries) rather than positional matches — when you need
positional checks, stabilize collections first by sorting children arrays by a
stable key (e.g., fetchId or kind) before asserting; reference res.Body and the
schedule tree keys like "children", "kind", and "fetch.fetchId" when locating
the code to change.
---
Outside diff comments:
In `@router/core/engine_loader_hooks.go`:
- Around line 177-178: The span is retrieved and ended unconditionally which can
prematurely close a parent span; wrap the span lifecycle and all span operations
inside an if f.tracingEnabled { ... } block: move the span :=
trace.SpanFromContext(ctx) and defer span.End() into that guard, and only call
span.RecordError(...), recordFetchError(..., span, ...), span.SetAttributes(...)
when f.tracingEnabled is true (or pass nil/skip calls otherwise). Update all
usages referenced (span variable, span.RecordError, recordFetchError invocations
that accept span, and span.SetAttributes) so they execute only when
f.tracingEnabled to avoid mutating/ending spans the code didn’t create.
---
Nitpick comments:
In `@router-tests/bench/engine-modes/lat-proxy/Dockerfile`:
- Around line 1-8: The Dockerfile currently leaves the final image running as
root because there is no USER instruction; to fix, in the final stage add
creation of a non-root user (e.g., adduser or addgroup + useradd) and set
ownership of the installed binary (/lat-proxy) to that user, then add a USER
<username> line before ENTRYPOINT so the container runs as the non-root user;
update the final stage that copies /lat-proxy to perform chown and create the
user so the binary is executable by that user.
- Line 1: The Dockerfile currently uses "FROM golang:1.25-alpine AS build" and
leaves out a USER directive so the container runs as root; update the Dockerfile
to create a non-root user (e.g., add a group and user with addgroup/adduser or
adduser -D) during the build stage, chown any application directories to that
user, and add a "USER <username>" directive in the final stage so the container
does not run as root; ensure the steps reference the existing "FROM
golang:1.25-alpine AS build" stage and the new "USER" directive to switch
context.
🪄 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: 3424a8e8-636c-435d-a037-9708da73aabe
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
router-tests/bench/engine-modes/README.mdrouter-tests/bench/engine-modes/byteid.shrouter-tests/bench/engine-modes/k6.jsrouter-tests/bench/engine-modes/lat-proxy/Dockerfilerouter-tests/bench/engine-modes/lat-proxy/main.gorouter-tests/bench/engine-modes/skew_bench.shrouter-tests/bench/engine-modes/skew_matrix.shrouter-tests/bench/engine-modes/uniform_bench.shrouter-tests/bench/engine-modes/uniform_matrix.shrouter-tests/go.modrouter-tests/operations/execution_plan_scheduling_test.gorouter-tests/testenv/testenv.gorouter/core/context.gorouter/core/engine_loader_hooks.gorouter/core/engine_loader_hooks_test.gorouter/core/executor.gorouter/core/graph_server.gorouter/core/operation_planner.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/config_test.gorouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
✅ Files skipped from review due to trivial changes (3)
- router/core/context.go
- router-tests/testenv/testenv.go
- router/pkg/config/testdata/config_full.json
🚧 Files skipped from review as they are similar to previous changes (9)
- router/pkg/config/testdata/config_defaults.json
- router/core/engine_loader_hooks_test.go
- router/pkg/config/config.schema.json
- router-tests/bench/engine-modes/README.md
- router/core/operation_planner.go
- router/pkg/config/config_test.go
- router/pkg/config/config.go
- router/core/graph_server.go
- router/core/executor.go
| } | ||
| }`, | ||
| }) | ||
| require.Equal(t, `{"data":{"products":[{"__typename":"Consultancy","lead":{"__typename":"Employee","id":1,"derivedMood":"HAPPY"},"isLeadAvailable":false},{"__typename":"Cosmo"},{"__typename":"SDK"}]},"extensions":{"queryPlan":{"version":"1","kind":"Sequence","children":[{"kind":"Single","fetch":{"kind":"Single","subgraphName":"employees","subgraphId":"0","fetchId":0,"query":"{\n products {\n __typename\n ... on Consultancy {\n lead {\n __typename\n id\n }\n __typename\n upc\n }\n }\n}"}},{"kind":"Parallel","children":[{"kind":"Sequence","children":[{"kind":"Single","fetch":{"kind":"BatchEntity","path":"products.@.lead","subgraphName":"availability","subgraphId":"5","fetchId":1,"dependsOnFetchIds":[0],"representations":[{"kind":"@key","typeName":"Employee","fragment":"fragment Key on Employee {\n __typename\n id\n}"}],"query":"query($representations: [_Any!]!){\n _entities(representations: $representations){\n ... on Employee {\n __typename\n isAvailable\n }\n }\n}","dependencies":[{"coordinate":{"typeName":"Employee","fieldName":"isAvailable"},"isUserRequested":false,"dependsOn":[{"fetchId":0,"subgraph":"employees","coordinate":{"typeName":"Employee","fieldName":"id"},"isKey":true,"isRequires":false}]}]}},{"kind":"Single","fetch":{"kind":"BatchEntity","path":"products","subgraphName":"employees","subgraphId":"0","fetchId":4,"dependsOnFetchIds":[0,1],"representations":[{"kind":"@requires","typeName":"Consultancy","fieldName":"isLeadAvailable","fragment":"fragment Requires_for_isLeadAvailable on Consultancy {\n lead {\n isAvailable\n }\n}"},{"kind":"@key","typeName":"Consultancy","fragment":"fragment Key on Consultancy {\n __typename\n upc\n}"}],"query":"query($representations: [_Any!]!){\n _entities(representations: $representations){\n ... on Consultancy {\n __typename\n isLeadAvailable\n }\n }\n}","dependencies":[{"coordinate":{"typeName":"Consultancy","fieldName":"isLeadAvailable"},"isUserRequested":true,"dependsOn":[{"fetchId":3,"subgraph":"employees","coordinate":{"typeName":"Consultancy","fieldName":"lead"},"isKey":false,"isRequires":true},{"fetchId":2,"subgraph":"mood","coordinate":{"typeName":"Consultancy","fieldName":"lead"},"isKey":false,"isRequires":true},{"fetchId":1,"subgraph":"availability","coordinate":{"typeName":"Consultancy","fieldName":"lead"},"isKey":false,"isRequires":true},{"fetchId":0,"subgraph":"employees","coordinate":{"typeName":"Consultancy","fieldName":"lead"},"isKey":false,"isRequires":true},{"fetchId":1,"subgraph":"availability","coordinate":{"typeName":"Employee","fieldName":"isAvailable"},"isKey":false,"isRequires":true},{"fetchId":0,"subgraph":"employees","coordinate":{"typeName":"Consultancy","fieldName":"upc"},"isKey":true,"isRequires":false}]}]}}]},{"kind":"Sequence","children":[{"kind":"Single","fetch":{"kind":"BatchEntity","path":"products.@.lead","subgraphName":"mood","subgraphId":"6","fetchId":2,"dependsOnFetchIds":[0],"representations":[{"kind":"@key","typeName":"Employee","fragment":"fragment Key on Employee {\n __typename\n id\n}"}],"query":"query($representations: [_Any!]!){\n _entities(representations: $representations){\n ... on Employee {\n __typename\n currentMood\n }\n }\n}","dependencies":[{"coordinate":{"typeName":"Employee","fieldName":"currentMood"},"isUserRequested":false,"dependsOn":[{"fetchId":0,"subgraph":"employees","coordinate":{"typeName":"Employee","fieldName":"id"},"isKey":true,"isRequires":false}]}]}},{"kind":"Single","fetch":{"kind":"BatchEntity","path":"products.@.lead","subgraphName":"employees","subgraphId":"0","fetchId":3,"dependsOnFetchIds":[0,2],"representations":[{"kind":"@requires","typeName":"Employee","fieldName":"derivedMood","fragment":"fragment Requires_for_derivedMood on Employee {\n currentMood\n}"},{"kind":"@key","typeName":"Employee","fragment":"fragment Key on Employee {\n __typename\n id\n}"}],"query":"query($representations: [_Any!]!){\n _entities(representations: $representations){\n ... on Employee {\n __typename\n derivedMood\n }\n }\n}","dependencies":[{"coordinate":{"typeName":"Employee","fieldName":"derivedMood"},"isUserRequested":true,"dependsOn":[{"fetchId":2,"subgraph":"mood","coordinate":{"typeName":"Employee","fieldName":"currentMood"},"isKey":false,"isRequires":true},{"fetchId":0,"subgraph":"employees","coordinate":{"typeName":"Employee","fieldName":"id"},"isKey":true,"isRequires":false}]}]}}]}]}],"normalizedQuery":"query Requires {products {__typename ... on Consultancy {lead {__typename id derivedMood} isLeadAvailable}}}"}}}`, res.Body) |
Contributor
There was a problem hiding this comment.
Avoid full raw-body snapshot equality for nested plan assertions.
require.Equal on the entire JSON body is brittle and can flake on non-semantic serialization/order changes (especially under nested Parallel children). Prefer require.JSONEq for structural JSON comparison, and assert only the required schedule-tree invariants (or stabilize ordering before positional checks).
As per coding guidelines, sort non-deterministic collections by a stable key before positional assertions.
🤖 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/operations/execution_plan_scheduling_test.go` at line 40, The
test currently does a brittle string equality on the entire response body
(require.Equal comparing the raw JSON in res.Body), which can flake due to
non-deterministic ordering in nested plan children; change this to use
structural JSON comparison and targeted assertions: replace the raw
require.Equal with require.JSONEq(expectedJSON, string(res.Body)) for structural
equality, then unmarshal res.Body into a map/struct and assert only the
essential schedule-tree invariants (e.g., presence of "Parallel"/"Sequence"
nodes and specific fetch entries) rather than positional matches — when you need
positional checks, stabilize collections first by sorting children arrays by a
stable key (e.g., fetchId or kind) before asserting; reference res.Body and the
schedule tree keys like "children", "kind", and "fetch.fetchId" when locating
the code to change.
Source: Coding guidelines
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
1 participant
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.
feat(router): engine flags for dataflow execution and execution plan scheduling
Router-side integration for the graphql-go-tools engine PR
(wundergraph/graphql-go-tools#1535), plus router hot-path perf
improvements and a self-contained engine-modes benchmark suite.
Depends on the engine PR. This PR pins graphql-go-tools to a pseudo-version
of that branch and stays in draft until it merges and the pin can move to a
tagged release.
What's in here (5 commits)
feat(router): engine flags
EngineExecutionConfiguration.EnableDataflowExecution(
ENGINE_ENABLE_DATAFLOW) andEnableExecutionPlanScheduling(
ENGINE_ENABLE_SCHEDULE_TREE), both default off, registered in the configJSON schema. The scheduler option is constructor-injected into
OperationPlanner(no env reads in planning code). Startup log lines proveenv → config → engine plumb-through; with both flags set, a warning
documents the safe degradation (the engine structurally rejects nested
schedule-tree plans in the dataflow executor and falls back).
TestExecutionPlanSchedulingPlumbingpins the full response of amulti-subgraph operation under the scheduler — config wiring and the nested
Parallel(Sequence...)plan shape proven end to end.perf(router): skip per-fetch telemetry when disabled; lazy-init request context keys
Engine loader hooks return early when tracing, metrics, and subgraph access
logging are all disabled (the default tracer was a NeverSample provider
that still allocated a span per fetch);
requestContext.keysallocateslazily on first
Set.test(router-tests): testenv env bridge — the whole suite re-runs per
engine mode via the same env names (harness only; production config is
constructor-injected).
bench(router-tests): engine-modes suite (
router-tests/bench/engine-modes/)Byte-identity gate + uniform/skew matrices for the three engine modes from
ONE router image, encoding the verified methodology (netem with qdisc
verification, CPU pinning, readiness gates, valid_bytes per row,
warmup-capped k6, interleaved median-of-3). Cosmo-only by design.
Evidence
Byte identity:
BYTEID GATE: PASS— one sha256 acrossbaseline/dataflow/scheduler/both, 50 requests per mode, exact 87,599 bytes.
Engine subset, 4 modes (
./security/... ./operations/...): green withdocumented exclusions only — redis-CLUSTER subtests (local infra; CI's
service network covers them) and the two by-design divergences under
non-default modes (
TestQueryPlansplan-shape under scheduler;TestSubgraphMergeResultsconflict-attribution family under dataflow).Full router-tests suite (flags off, -race): failures are exclusively
pre-existing environment classes reproduced bit-identically on upstream
mainon the same host (Kafka/NATS dev-stack config, redis-cluster, OCIplugin infra, named
TestFlaky*).Playground: query-plan Tree and Text views render the nested schedule
tree (Parallel root, 13 fetch nodes) with zero console errors; ART trace
view unaffected.
Benchmarks (M4 Max, OrbStack, 4-CPU-pinned gateway, median-of-3
interleaved):
Defaults
Both flags off ⇒ zero behavior change: config goldens updated only by the two
new fields (
false), byte-identity proven, flags-off A/B vs theupstream-pinned build within noise.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Performance
Tests
Documentation