feat: Phase 0 baseline benchmark script and log (WP-00) (#87) #95

Claude · 2026-03-13T05:04:49Z

Claude commented

2026-03-13 05:04:49 +00:00

Closes #87

Summary

Adds benchmarks/benchmark_renders.sh — shell script that times 10 sequential card renders via curl against any API environment (configurable via API_BASE and CARD_TYPE env vars). Auto-fetches player IDs from the live cardset or accepts explicit IDs as arguments.
Adds benchmarks/BASELINE.md — documents the methodology for both render-time and batch-upload benchmarks, provides result tables for pre/post optimization comparison, and tracks expected improvement targets across WP-01 through WP-04.

Files Changed

benchmarks/benchmark_renders.sh (new, executable)
benchmarks/BASELINE.md (new)

Notes

The S3 batch upload benchmark (via pd-cards upload in the card-creation repo) is documented in BASELINE.md with instructions, but requires running in that separate repo. The database repo's work item is the render-timing script only.
Actual baseline numbers in BASELINE.md are marked _TBD_ — they need to be populated by running the script against pddev.manticorum.com:816 before WP-01/WP-02 land.
Results are appended to benchmarks/render_timings.txt (gitignored-by-default — add to .gitignore if desired, or commit the results file once populated).

Test Results

No automated test suite. Verified by reading back the created files.

Closes #87 ## Summary - Adds `benchmarks/benchmark_renders.sh` — shell script that times 10 sequential card renders via `curl` against any API environment (configurable via `API_BASE` and `CARD_TYPE` env vars). Auto-fetches player IDs from the live cardset or accepts explicit IDs as arguments. - Adds `benchmarks/BASELINE.md` — documents the methodology for both render-time and batch-upload benchmarks, provides result tables for pre/post optimization comparison, and tracks expected improvement targets across WP-01 through WP-04. ## Files Changed - `benchmarks/benchmark_renders.sh` (new, executable) - `benchmarks/BASELINE.md` (new) ## Notes - The S3 batch upload benchmark (via `pd-cards upload` in the `card-creation` repo) is documented in `BASELINE.md` with instructions, but requires running in that separate repo. The `database` repo's work item is the render-timing script only. - Actual baseline numbers in `BASELINE.md` are marked `_TBD_` — they need to be populated by running the script against `pddev.manticorum.com:816` before WP-01/WP-02 land. - Results are appended to `benchmarks/render_timings.txt` (gitignored-by-default — add to `.gitignore` if desired, or commit the results file once populated). ## Test Results No automated test suite. Verified by reading back the created files.

Claude added 1 commit 2026-03-13 05:04:50 +00:00

feat: add Phase 0 baseline benchmark script and log (WP-00) (#87 ) 4f2a66b67e

Closes #87

Add benchmarks/benchmark_renders.sh to time 10 sequential card renders
via curl against any API environment, and benchmarks/BASELINE.md to
record methodology and results for pre/post optimization comparison.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Claude referenced this pull request

2026-03-13 05:05:00 +00:00

WP-00: Baseline Benchmarks #87

Claude added the

ai-reviewing

label 2026-03-13 05:15:51 +00:00

Claude commented

2026-03-13 05:17:47 +00:00

AI Code Review

Files Reviewed

benchmarks/benchmark_renders.sh (new, executable)
benchmarks/BASELINE.md (new)

Findings

Correctness

BUG — curl -w override breaks timing collection (benchmark_renders.sh, lines 57–61)

HTTP_CODE=$(curl -s -o /dev/null \
    --write-out "%{http_code}" \
    -w " %{time_total}" \
    "$URL" 2>&1)

--write-out and -w are the same curl option (long/short forms). Per curl docs, when the same option is specified twice, the last value wins. So HTTP_CODE will contain only the timing value (e.g., " 2.345123") — the HTTP status code is discarded.

Downstream consequences:

STATUS=$(echo "$HTTP_CODE" | awk '{print $1}') → captures the timing, not the HTTP code
TIMING=$(echo "$HTTP_CODE" | awk '{print $2}') → always empty
if [ "$STATUS" = "200" ] → never true
$COUNT stays 0, script always outputs "No successful renders — check API_BASE and player IDs"

The script cannot collect any baseline timings in its current form.

Fix: combine into a single format string:

HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code} %{time_total}" "$URL")

Missing ?nocache=1 parameter (benchmark_renders.sh, lines 53–55)

The inline comment says:

# Bypass cached PNG files by adding ?nocache=1 (treated as unknown param, triggers re-render)
# Remove this flag after baseline is captured to test cache-hit performance separately.

But ?nocache=1 is not actually appended to $URL. If the endpoint has file-based caching (the players router generates PNG files to disk), repeated runs after the first will measure cache-hit latency rather than full render time. The "baseline" would then not represent the optimization problem being solved.

Security

No issues. No credentials, no injection surfaces, all variable expansions are quoted.

Style & Conventions

No significant issues. set -euo pipefail is good defensive practice throughout.

Suggestions

Duplicate row in BASELINE.md Section 2 (Batch Upload table): Both data rows are identical (| dev | 20 | _TBD_ | _TBD_ |). Likely a copy-paste error — presumably meant to cover two different conditions (e.g., batting vs pitching, or two separate runs for variance).
render_timings.txt not gitignored: The PR notes describe it as "gitignored-by-default" but .txt files are not covered by .gitignore. The output file will appear as an untracked file and could be accidentally committed. Consider adding benchmarks/render_timings.txt to .gitignore in this PR.

Verdict: REQUEST_CHANGES

The curl -w override bug (finding #1) means the script cannot collect any successful render timings — every run exits with "No successful renders." Since WP-00's sole purpose is capturing baseline numbers before the optimization work lands, this must be fixed before merging. The missing ?nocache=1 should also be addressed so the baselines accurately reflect cold-render time.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `benchmarks/benchmark_renders.sh` (new, executable) - `benchmarks/BASELINE.md` (new) ### Findings #### Correctness **BUG — curl `-w` override breaks timing collection (benchmark_renders.sh, lines 57–61)** ```bash HTTP_CODE=$(curl -s -o /dev/null \ --write-out "%{http_code}" \ -w " %{time_total}" \ "$URL" 2>&1) ``` `--write-out` and `-w` are the same curl option (long/short forms). Per curl docs, when the same option is specified twice, the last value wins. So `HTTP_CODE` will contain only the timing value (e.g., `" 2.345123"`) — the HTTP status code is discarded. Downstream consequences: - `STATUS=$(echo "$HTTP_CODE" | awk '{print $1}')` → captures the timing, not the HTTP code - `TIMING=$(echo "$HTTP_CODE" | awk '{print $2}')` → always empty - `if [ "$STATUS" = "200" ]` → never true - `$COUNT` stays 0, script always outputs "No successful renders — check API_BASE and player IDs" The script cannot collect any baseline timings in its current form. **Fix:** combine into a single format string: ```bash HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code} %{time_total}" "$URL") ``` **Missing `?nocache=1` parameter (benchmark_renders.sh, lines 53–55)** The inline comment says: ``` # Bypass cached PNG files by adding ?nocache=1 (treated as unknown param, triggers re-render) # Remove this flag after baseline is captured to test cache-hit performance separately. ``` But `?nocache=1` is not actually appended to `$URL`. If the endpoint has file-based caching (the players router generates PNG files to disk), repeated runs after the first will measure cache-hit latency rather than full render time. The "baseline" would then not represent the optimization problem being solved. #### Security No issues. No credentials, no injection surfaces, all variable expansions are quoted. #### Style & Conventions No significant issues. `set -euo pipefail` is good defensive practice throughout. #### Suggestions - **Duplicate row in BASELINE.md Section 2 (Batch Upload table):** Both data rows are identical (`| dev | 20 | _TBD_ | _TBD_ |`). Likely a copy-paste error — presumably meant to cover two different conditions (e.g., batting vs pitching, or two separate runs for variance). - **`render_timings.txt` not gitignored:** The PR notes describe it as "gitignored-by-default" but `.txt` files are not covered by `.gitignore`. The output file will appear as an untracked file and could be accidentally committed. Consider adding `benchmarks/render_timings.txt` to `.gitignore` in this PR. ### Verdict: REQUEST_CHANGES The curl `-w` override bug (finding #1) means the script cannot collect any successful render timings — every run exits with "No successful renders." Since WP-00's sole purpose is capturing baseline numbers before the optimization work lands, this must be fixed before merging. The missing `?nocache=1` should also be addressed so the baselines accurately reflect cold-render time. --- *Automated review by Claude PR Reviewer*

Claude added the

ai-changes-requested

label 2026-03-13 05:18:25 +00:00

Claude added the

ai-working

label 2026-03-13 05:30:44 +00:00

cal referenced this issue from a commit

2026-03-13 05:31:43 +00:00

fix: address review feedback (#95)

cal added 1 commit 2026-03-13 05:31:44 +00:00

fix: address review feedback (#95 ) 2ab6e71735

- Fix curl -w override bug: consolidate --write-out/"-w" into single
  -w "%{http_code} %{time_total}" so STATUS and TIMING are both captured
- Add ?nocache=1 to render URL so baseline measures cold render time
- Fix duplicate BASELINE.md Section 2 rows (batting vs pitching)
- Add benchmarks/render_timings.txt to .gitignore

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Claude commented

2026-03-13 05:31:52 +00:00

Review Feedback Addressed

All four findings from the REQUEST_CHANGES review have been fixed in commit 2ab6e71.

Changes made

1. Fixed curl -w override bug (critical)
Consolidated --write-out "%{http_code}" + -w " %{time_total}" into a single -w "%{http_code} %{time_total}". The two flags were aliases and the last one won, discarding the HTTP status code. STATUS and TIMING now parse correctly from the combined output.

2. Added ?nocache=1 to render URL
The URL now actually appends ?nocache=1 as the comment described. Ensures baseline measures cold Playwright render time, not cache-hit latency.

3. Fixed duplicate BASELINE.md Section 2 rows
Differentiated the two identical | dev | 20 | ... rows as dev (batting) and dev (pitching), matching the structure of Section 1.

4. Added benchmarks/render_timings.txt to .gitignore
Prevents the benchmark output file from appearing as untracked and being accidentally committed.

Automated fix by Claude issue-worker

## Review Feedback Addressed All four findings from the REQUEST_CHANGES review have been fixed in commit `2ab6e71`. ### Changes made **1. Fixed curl `-w` override bug (critical)** Consolidated `--write-out "%{http_code}"` + `-w " %{time_total}"` into a single `-w "%{http_code} %{time_total}"`. The two flags were aliases and the last one won, discarding the HTTP status code. `STATUS` and `TIMING` now parse correctly from the combined output. **2. Added `?nocache=1` to render URL** The URL now actually appends `?nocache=1` as the comment described. Ensures baseline measures cold Playwright render time, not cache-hit latency. **3. Fixed duplicate BASELINE.md Section 2 rows** Differentiated the two identical `| dev | 20 | ...` rows as `dev (batting)` and `dev (pitching)`, matching the structure of Section 1. **4. Added `benchmarks/render_timings.txt` to `.gitignore`** Prevents the benchmark output file from appearing as untracked and being accidentally committed. --- *Automated fix by Claude issue-worker*

Claude removed the

ai-working

ai-changes-requested

labels 2026-03-13 05:32:06 +00:00

cal requested changes 2026-03-13 13:14:16 +00:00

Dismissed

cal left a comment

AI Code Review

Files Reviewed

benchmarks/benchmark_renders.sh (added, executable)
benchmarks/BASELINE.md (added)
.gitignore (modified)

Findings

Correctness

1. stderr redirect swallows curl errors into timing variables (benchmark_renders.sh, line 52)

HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code} %{time_total}" "$URL" 2>&1)

The 2>&1 redirect causes curl error messages (e.g., "curl: (7) Failed to connect") to be captured in $HTTP_CODE. Since set -euo pipefail is active, if curl exits non-zero the script will abort at this line — which is arguably fine — but the redirect is misleading: curl's -s flag already silences progress output, so 2>&1 only serves to absorb actual error messages that would help diagnose failures. The redirect should be removed. If curl fails, set -e will exit, and the user will see nothing. Better to let stderr through so the error is visible:

HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code} %{time_total}" "$URL")

Alternatively, use || true with explicit error handling if you want to continue on a failed render.

2. ?nocache=1 likely does not bypass the nginx cache (benchmark_renders.sh, line 49)

The script appends ?nocache=1 to the card URL with the intent of bypassing cached PNG files. However, the nginx caching layer for this API uses ?d={year}-{month}-{day}-{timestamp} as the cache-busting mechanism (per CLAUDE.md in the card-creation repo). The parameter nocache=1 is almost certainly not in the nginx proxy_cache_key config and will be silently ignored — meaning the script may be measuring cache-hit latency (~milliseconds) rather than full Playwright render times (~2-3s). This undermines the entire purpose of the benchmark.

To actually force a fresh render, use a unique timestamp per request:

CACHE_BUST=$(date +%s%3N)
URL="$API_BASE/api/v2/players/$player_id/${CARD_TYPE}card?d=$(date +%Y-%-m-%-d)-${CACHE_BUST}"

This matches the established cache-busting convention and is guaranteed to produce a cache miss.

3. Incorrect pd-cards upload command in BASELINE.md (BASELINE.md, line 53)

time pd-cards upload --cardset 24 --limit 20

Per the card-creation repo's CLAUDE.md, the correct syntax for the upload command is:

pd-cards upload s3 --cardset "2025 Live" --limit 20

upload is a subcommand group requiring the s3 subcommand, and --cardset takes the cardset name string (e.g., "2025 Live"), not a numeric ID. The command as written will fail with a CLI error. The LIVE_CARDSET_ID from CLAUDE.md is 24, so whoever runs this will be confused when the command rejects the integer argument.

Security

No issues. The script accepts only player IDs from the API or explicit CLI arguments. No user-controlled input reaches a shell eval path. API credentials are not handled.

Style & Conventions

The .gitignore entry is clean.
The OUTFILE path using dirname "$0" is correct — it writes relative to the script file, not the caller's CWD.
The PLAYER_IDS=($(echo "$RAW" | jq ...)) word-split pattern is acceptable for numeric IDs; no real risk here.
The jq selector chain .players[].id // .[]?.id // .[] is pragmatic but fragile — if the API response shape changes, it fails silently (caught only by the length check). A comment explaining the expected response shape would help.

Suggestions

Consider adding a shebang comment block noting the actual cache-busting parameter format alongside the ?nocache=1 caveat, so future readers know why the URL format changed.
The BASELINE.md progress table is clean and well-structured. The TBD entries are intentional and clearly called out. No changes needed there.
It would be useful to note in the script header (or BASELINE.md) that the dev API is at port 816 (matching CLAUDE.md) as the default API_BASE example — right now the default is localhost:8000 which is the container-internal port.

Verdict: REQUEST_CHANGES

Two issues need fixing before merge:

2>&1 on the curl timing line (line 52 of the script) — remove the redirect so errors surface to the user instead of being swallowed into the HTTP code variable.
?nocache=1 does not bypass the nginx cache (line 49) — replace with the established ?d=... timestamp pattern or the benchmark will silently measure cache hits instead of full renders, producing misleading baseline numbers.

The pd-cards upload command in BASELINE.md also needs correcting (add s3 subcommand and use cardset name instead of ID), though that is a documentation-only fix.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `benchmarks/benchmark_renders.sh` (added, executable) - `benchmarks/BASELINE.md` (added) - `.gitignore` (modified) --- ### Findings #### Correctness **1. `stderr` redirect swallows curl errors into timing variables (benchmark_renders.sh, line 52)** ```bash HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code} %{time_total}" "$URL" 2>&1) ``` The `2>&1` redirect causes curl error messages (e.g., "curl: (7) Failed to connect") to be captured in `$HTTP_CODE`. Since `set -euo pipefail` is active, if curl exits non-zero the script will abort at this line — which is arguably fine — but the redirect is misleading: curl's `-s` flag already silences progress output, so `2>&1` only serves to absorb actual error messages that would help diagnose failures. The redirect should be removed. If curl fails, `set -e` will exit, and the user will see nothing. Better to let stderr through so the error is visible: ```bash HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code} %{time_total}" "$URL") ``` Alternatively, use `|| true` with explicit error handling if you want to continue on a failed render. **2. `?nocache=1` likely does not bypass the nginx cache (benchmark_renders.sh, line 49)** The script appends `?nocache=1` to the card URL with the intent of bypassing cached PNG files. However, the nginx caching layer for this API uses `?d={year}-{month}-{day}-{timestamp}` as the cache-busting mechanism (per CLAUDE.md in the card-creation repo). The parameter `nocache=1` is almost certainly not in the nginx `proxy_cache_key` config and will be silently ignored — meaning the script may be measuring cache-hit latency (~milliseconds) rather than full Playwright render times (~2-3s). This undermines the entire purpose of the benchmark. To actually force a fresh render, use a unique timestamp per request: ```bash CACHE_BUST=$(date +%s%3N) URL="$API_BASE/api/v2/players/$player_id/${CARD_TYPE}card?d=$(date +%Y-%-m-%-d)-${CACHE_BUST}" ``` This matches the established cache-busting convention and is guaranteed to produce a cache miss. **3. Incorrect `pd-cards upload` command in BASELINE.md (BASELINE.md, line 53)** ```bash time pd-cards upload --cardset 24 --limit 20 ``` Per the card-creation repo's CLAUDE.md, the correct syntax for the upload command is: ```bash pd-cards upload s3 --cardset "2025 Live" --limit 20 ``` `upload` is a subcommand group requiring the `s3` subcommand, and `--cardset` takes the cardset name string (e.g., `"2025 Live"`), not a numeric ID. The command as written will fail with a CLI error. The `LIVE_CARDSET_ID` from CLAUDE.md is `24`, so whoever runs this will be confused when the command rejects the integer argument. #### Security No issues. The script accepts only player IDs from the API or explicit CLI arguments. No user-controlled input reaches a shell eval path. API credentials are not handled. #### Style & Conventions - The `.gitignore` entry is clean. - The `OUTFILE` path using `dirname "$0"` is correct — it writes relative to the script file, not the caller's CWD. - The `PLAYER_IDS=($(echo "$RAW" | jq ...))` word-split pattern is acceptable for numeric IDs; no real risk here. - The `jq` selector chain `.players[].id // .[]?.id // .[]` is pragmatic but fragile — if the API response shape changes, it fails silently (caught only by the length check). A comment explaining the expected response shape would help. #### Suggestions - Consider adding a shebang comment block noting the actual cache-busting parameter format alongside the `?nocache=1` caveat, so future readers know why the URL format changed. - The `BASELINE.md` progress table is clean and well-structured. The _TBD_ entries are intentional and clearly called out. No changes needed there. - It would be useful to note in the script header (or BASELINE.md) that the dev API is at port 816 (matching CLAUDE.md) as the default `API_BASE` example — right now the default is `localhost:8000` which is the container-internal port. --- ### Verdict: REQUEST_CHANGES Two issues need fixing before merge: 1. **`2>&1` on the curl timing line** (line 52 of the script) — remove the redirect so errors surface to the user instead of being swallowed into the HTTP code variable. 2. **`?nocache=1` does not bypass the nginx cache** (line 49) — replace with the established `?d=...` timestamp pattern or the benchmark will silently measure cache hits instead of full renders, producing misleading baseline numbers. The `pd-cards upload` command in BASELINE.md also needs correcting (add `s3` subcommand and use cardset name instead of ID), though that is a documentation-only fix. --- *Automated review by Claude PR Reviewer*

benchmarks/BASELINE.md

						
				@ -0,0 +50,4 @@

				**What is measured:** Wall-clock time to render and upload N card images to S3

				using the `pd-cards upload` CLI (in the `card-creation` repo).

				### Method

cal commented

2026-03-13 13:14:16 +00:00

This command is incorrect. The pd-cards upload subcommand requires the s3 sub-subcommand, and --cardset takes a name string, not a numeric ID. Correct form:

time pd-cards upload s3 --cardset "2025 Live" --limit 20

Running the command as written will produce a CLI error.

This command is incorrect. The `pd-cards upload` subcommand requires the `s3` sub-subcommand, and `--cardset` takes a name string, not a numeric ID. Correct form: ```bash time pd-cards upload s3 --cardset "2025 Live" --limit 20 ``` Running the command as written will produce a CLI error.

benchmarks/benchmark_renders.sh

						
				@ -0,0 +46,4 @@

				# --- Run renders ---

				TOTAL=0

				COUNT=0

cal commented

2026-03-13 13:14:16 +00:00

?nocache=1 is almost certainly not in the nginx proxy_cache_key and will be silently ignored, meaning this measures cache-hit latency (~ms) not full Playwright render time (~2-3s). Use the established cache-busting format instead:

URL="$API_BASE/api/v2/players/$player_id/${CARD_TYPE}card?d=$(date +%Y-%-m-%-d)-$(date +%s)"

This matches the ?d={year}-{month}-{day}-{timestamp} convention from the card-creation codebase and guarantees a cache miss per request.

`?nocache=1` is almost certainly not in the nginx `proxy_cache_key` and will be silently ignored, meaning this measures cache-hit latency (~ms) not full Playwright render time (~2-3s). Use the established cache-busting format instead: ```bash URL="$API_BASE/api/v2/players/$player_id/${CARD_TYPE}card?d=$(date +%Y-%-m-%-d)-$(date +%s)" ``` This matches the `?d={year}-{month}-{day}-{timestamp}` convention from the card-creation codebase and guarantees a cache miss per request.

benchmarks/benchmark_renders.sh

						
				@ -0,0 +49,4 @@

				for player_id in "${PLAYER_IDS[@]}"; do

				    # Bypass cached PNG files; remove ?nocache=1 after baseline is captured to test cache-hit performance.

				    URL="$API_BASE/api/v2/players/$player_id/${CARD_TYPE}card?nocache=1"

cal commented

2026-03-13 13:14:16 +00:00

The 2>&1 redirect here causes curl error messages to be captured in $HTTP_CODE rather than appearing on stderr. Since curl -s already suppresses progress output, the only thing 2>&1 captures is actual error text (e.g. connection refused). With set -e active, a curl failure will abort the script — but silently, because the error was swallowed. Remove 2>&1 so errors surface to the terminal.

The `2>&1` redirect here causes curl error messages to be captured in `$HTTP_CODE` rather than appearing on stderr. Since `curl -s` already suppresses progress output, the only thing `2>&1` captures is actual error text (e.g. connection refused). With `set -e` active, a curl failure will abort the script — but silently, because the error was swallowed. Remove `2>&1` so errors surface to the terminal.