Repository navigation
ci(node): add paired SDK performance benchmarks - #5453
dustinbyrne wants to merge 2 commits into
Conversation
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
stamphog declined to auto-review this pull request because three gates failed. The deny-list gate matched infra_cicd, since the PR adds .github/workflows/node-benchmarks.yml. The size gate found 815 substantive lines against an 800-line ceiling, and the tier gate classified the change as T2-never (two areas, CI). These are policy refusals and say nothing about the quality of the work.
Please ask a human reviewer to look at it. Splitting the workflow file from the benchmark code under packages/node/benchmarks/ would bring each part under the size ceiling, but the workflow change would still need human review.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: infra_cicd |
| size | ✗ | too large for auto-review (815L substantive in global — ceiling is 800L; 815L, 7F total, 1171L/9F incl. docs/generated/snapshots) |
| tier | ✗ | classified as T2-never: T2-never (1171L, 9F, two-areas, ci) |
| stamphog 2.6.1 | .stamphog/policy.yml @ unknown · reviewed head f726795 |
|
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
PostHog Review · Standard · reviewed f726795: 1 Must fix, 1 Should fix.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
| @@ -0,0 +1,256 @@ | |||
| import assert from 'node:assert/strict' | |||
| import { test } from 'node:test' | |||
There was a problem hiding this comment.
P1 · Exclude the Node benchmark tests from Vitest discovery
Running test:unit or test:edge-compat now discovers this file through Vitest's default glob, since the shared config does not exclude benchmarks/**. Its node:test registrations create no Vitest tests, so the existing suites fail with No test suite found. Exclude the benchmark directory from the shared Vitest configuration.
| await save() | ||
| } | ||
| } | ||
| if (records.base.status !== 'ok' || records.head.status !== 'ok') break |
There was a problem hiding this comment.
P2 · Bound repeated worker timeouts within the CI job budget
When full-size single-flag workloads hang on one revision, main breaks only the repetition loop and continues through every workload. Nine cases with two passes each consume 36 minutes in worker timeouts, exceeding the workflow's 30-minute limit and preventing diagnostic artifact upload. Stop after repeated timeouts or enforce a suite deadline that reserves upload time.
Coherence0 spec problems · 0 failing chokepoints · 2 spec gaps · 0 guard failures 44 components, 7 bullets (5 invariants, 2 requirements), 0 problems Chokepoints: none declared. Spec gaps: 2 entrances with outside or unknown trust and no traced control, and 10 more held by the adoption baseline
Close one with |
Node SDK compliance v2Commit: 94a1999 Capture v0 — acceptance✅ 28 passed / ❌ 3 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: Harness exit: 1; report validation exit: 1. Capture v1 — acceptance✅ 28 passed / ❌ 3 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: Harness exit: 1; report validation exit: 1. Capture v0 — migration✅ 55 passed / ❌ 1 non-passing / Drill-down: run artifacts → ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. Capture v1 — migration✅ 120 passed / ❌ 1 non-passing / Drill-down: run artifacts → ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. |
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
The code looks good; no qualifying findings. Verdict: correct.
Problem
SDK changes need reproducible PR-local evidence for flag evaluation time, allocation churn, and memory amplification, rather than relying on anecdotes or retained heap alone.
Changes
Shared report rendering and commenting will integrate in a stacked follow-up.
Validation
Release info
No version bump or changeset: benchmark/CI tooling only.
Agent context
Autonomy: Human-driven (agent-assisted).
Implemented using Pi, with a fresh read-only reviewer and local measurement controls. Separate passes keep profiler overhead out of timing; sampled allocation bytes and observed/lifetime RSS retain explicit scope and quality.