Skip to content

chore: add non-cached benchmark to find the price of costs control#2929

Merged
ysmolski merged 9 commits into
mainfrom
yury/add_non_cached_benchmarks_for_costs
Jun 11, 2026
Merged

chore: add non-cached benchmark to find the price of costs control#2929
ysmolski merged 9 commits into
mainfrom
yury/add_non_cached_benchmarks_for_costs

Conversation

@ysmolski

@ysmolski ysmolski commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

We already have two benchmark that use router with caching enabled. That does not make a good indicator of the delta when we enable the cost control feature.

This PR adds two benchmarks where caching is disabled and we could see the real price of Cost Control:

$ go test -bench=BenchmarkSequentialBigNotCached -benchmem -count=20 -run=^$ . > new.txt
$ benchstat -col .name new.txt
goos: darwin
goarch: arm64
pkg: github.com/wundergraph/cosmo/router-tests/protocol
cpu: Apple M4 Max
 │ SequentialBigNotCached │ SequentialBigNotCachedCostControl  │
 │         sec/op         │   sec/op     vs base               │
*-14              1.092m ± 1%   1.125m ± 0%  +3.05% (p=0.003 n=20)

 │ SequentialBigNotCached │  SequentialBigNotCachedCostControl  │
 │          B/s           │     B/s       vs base               │
*-14             2.952Mi ± 1%   2.871Mi ± 0%  -2.75% (p=0.003 n=20)

 │ SequentialBigNotCached │  SequentialBigNotCachedCostControl  │
 │          B/op          │     B/op      vs base               │
*-14             998.0Ki ± 0%   986.5Ki ± 0%  -1.15% (p=0.000 n=20)

 │ SequentialBigNotCached │ SequentialBigNotCachedCostControl  │
 │       allocs/op        │  allocs/op   vs base               │
*-14              11.15k ± 0%   11.22k ± 0%  +0.66% (p=0.000 n=20)

It shows that Cost Control adds ~80 allocations, 3% of CPU time when caching is disabled.

Summary by CodeRabbit

  • Tests
    • Updated cost-control benchmarks to use improved size estimates, expose cost headers, and validate estimated and actual cost values.
    • Refined benchmark timing so assertion and metric verification (including cache metric checks) are excluded from measured execution time.
  • New Features
    • Added a configurable load-test for large GraphQL responses with environment-driven parameters and runtime checks.
    • Added a test routing subgraph entry.
  • Documentation
    • Clarified profiling guide wording and example output.

We already have two benchmark that use router with caching enabled.
That does not make a good indicator of the delta when we enable
the cost control feature.

This PR adds two benchmarks where caching is disabled and we could see
the real price of Cost Control:

    $ go test -bench=BenchmarkSequentialBigNotCached -benchmem -count=20 -run=^$ . > new.txt
    $ benchstat -col .name new.txt
    goos: darwin
    goarch: arm64
    pkg: github.com/wundergraph/cosmo/router-tests/protocol
    cpu: Apple M4 Max
	 │ SequentialBigNotCached │ SequentialBigNotCachedCostControl  │
	 │         sec/op         │   sec/op     vs base               │
    *-14              1.092m ± 1%   1.125m ± 0%  +3.05% (p=0.003 n=20)

	 │ SequentialBigNotCached │  SequentialBigNotCachedCostControl  │
	 │          B/s           │     B/s       vs base               │
    *-14             2.952Mi ± 1%   2.871Mi ± 0%  -2.75% (p=0.003 n=20)

	 │ SequentialBigNotCached │  SequentialBigNotCachedCostControl  │
	 │          B/op          │     B/op      vs base               │
    *-14             998.0Ki ± 0%   986.5Ki ± 0%  -1.15% (p=0.000 n=20)

	 │ SequentialBigNotCached │ SequentialBigNotCachedCostControl  │
	 │       allocs/op        │  allocs/op   vs base               │
    *-14              11.15k ± 0%   11.22k ± 0%  +0.66% (p=0.000 n=20)

It shows that costs adds ~80 allocations, 3% of CPU time
when caching is disabled.
@ysmolski ysmolski requested a review from a team as a code owner June 8, 2026 14:15

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-a1077b3e962299532fab0155513e659279027fed

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Pause benchmark timing around assertions and run cache-metric checks off-timer; reconfigure sequential cost-control benchmarks to expose headers and validate estimated/actual cost per iteration. Add a k6 big-response benchmark script, add a local test1 subgraph to the router schema, and reflow profiling docs.

Changes

Benchmark Measurement and Cost-Control Validation

Layer / File(s) Summary
Benchmark timer control infrastructure
router-tests/testenv/testenv.go
Bench pauses timing after invoking the benchmark callback to run optional AssertCacheMetrics, then restarts timing before returning.
Cost-control benchmark configuration
router-tests/protocol/integration_test.go
Sequential cost-control benchmarks set EstimatedListSize: 5 and ExposeHeaders: true. An inline comment notes no need to stop the timer in BenchmarkSequentialBig.
Per-iteration assertion timing and header checks
router-tests/protocol/integration_test.go
BenchmarkSequentialBigNotCachedCostControl stops the timer each iteration before asserting response body size and validating CostEstimatedHeader and CostActualHeader, then restarts timing.

Load Test Script, Schema, and Docs

Layer / File(s) Summary
k6 big-response benchmark
router/bench-big-response.js
Adds a k6 script with env-driven options, constructs a large GraphQL Big query and request body, sets headers, posts per-iteration, and checks for HTTP 200 and absence of GraphQL errors when not discarding responses.
Graph schema subgraph
router/__schemas/graph.yaml
Adds a subgraphs entry test1 routing to http://localhost:4006/graphql.
Profiling docs reflow
router/docs/Profiling.md
Reflows and rewraps pprof guidance and replaces the k6 output example; minor text reformatting and removal of one sentence about allocations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • wundergraph/cosmo#2925: Related to changes that affect computation and validation of CostEstimatedHeader/CostActualHeader.
  • wundergraph/cosmo#2716: Related to earlier adjustments in how costs are calculated and validated in benchmarks.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main objective of the PR: adding benchmarks to measure the performance cost of the Cost Control feature without caching.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.17%. Comparing base (b09d547) to head (4e58318).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2929      +/-   ##
==========================================
- Coverage   66.38%   66.17%   -0.22%     
==========================================
  Files         258      258              
  Lines       27539    27539              
==========================================
- Hits        18282    18223      -59     
- Misses       7805     7853      +48     
- Partials     1452     1463      +11     

see 13 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-tests/protocol/integration_test.go`:
- Around line 1603-1614: The benchmark loop in Benchmark... (the for b.Loop()
block that issues xEnv.MakeGraphQLRequestOK with bigEmployeesQuery) is measuring
assertion overhead because the body-size check and header assertions run while
the timer is active; stop the benchmark timer before validating res.Body and the
header checks (b.StopTimer()), perform the len(res.Body) validation and the
require.Equal checks for core.CostEstimatedHeader and core.CostActualHeader,
then restart the timer with b.StartTimer() so only request/processing time is
measured; locate the assertions inside the for b.Loop() in the benchmark and
wrap them with b.StopTimer()/b.StartTimer() around the validations.
🪄 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: c7ac9107-18df-4b43-8300-32827dd74962

📥 Commits

Reviewing files that changed from the base of the PR and between 8636572 and 3bd2fd1.

📒 Files selected for processing (2)
  • router-tests/protocol/integration_test.go
  • router-tests/testenv/testenv.go

Comment thread router-tests/protocol/integration_test.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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/bench-big-response.js`:
- Line 77: Replace the substring check in the 'no graphql errors' response
assertion with proper JSON parsing: in the arrow function keyed 'no graphql
errors' parse r.body (try/catch) into an object and then assert r.status === 200
and (parsed.errors === undefined || (Array.isArray(parsed.errors) &&
parsed.errors.length === 0)); if JSON.parse throws or parsed.errors indicates
problems, return false (or DISCARD || false) so we don't misclassify responses.
Ensure you still short-circuit with the existing DISCARD condition and reference
the same function/arrow expression handling r.
- Around line 71-79: Wrap the HTTP request and subsequent check calls inside a
try/catch block in the default exported function: perform const res =
http.post(URL, body, { headers }) and the existing check(...) inside try, and in
catch catch any runtime error, log the error (console.error or a logger) and
report a controlled failed check (e.g., call check with a single failing
assertion so the iteration is recorded as failed instead of throwing). Ensure
references to res, http.post, check, DISCARD, URL, body and headers are guarded
so the catch path does not rethrow.

In `@router/docs/Profiling.md`:
- Line 110: Replace the non-ASCII “smart” single quotes around the pprof URL in
the documentation line containing go tool pprof
‘http://localhost:6060/debug/pprof/profile?seconds=5’ with plain ASCII quotes
(either ' or ") so the shell can execute the command; update the documentation
text to use go tool pprof 'http://localhost:6060/debug/pprof/profile?seconds=5'
(or with double quotes) wherever that exact pprof invocation appears.
- Around line 33-51: Add a language identifier to the unlabeled fenced code
block in router/docs/Profiling.md (the block that starts with "✓ is status 200")
to satisfy markdownlint MD040; update the opening fence from ``` to ```text so
the block becomes ```text ... ``` and leave the content unchanged.
🪄 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: 88adac9b-ce82-43cd-b15a-abc75c0910b8

📥 Commits

Reviewing files that changed from the base of the PR and between f619081 and 4e8b384.

📒 Files selected for processing (3)
  • router/__schemas/graph.yaml
  • router/bench-big-response.js
  • router/docs/Profiling.md
✅ Files skipped from review due to trivial changes (1)
  • router/__schemas/graph.yaml

Comment thread router/bench-big-response.js
Comment thread router/bench-big-response.js
Comment thread router/docs/Profiling.md
Comment thread router/docs/Profiling.md Outdated
@ysmolski ysmolski merged commit 1e70ce8 into main Jun 11, 2026
37 checks passed
@ysmolski ysmolski deleted the yury/add_non_cached_benchmarks_for_costs branch June 11, 2026 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants