feat(o11y): stamp testhubBuildUuid + buildProductMap on cypress build caps [SDK-7285] - #1180
feat(o11y): stamp testhubBuildUuid + buildProductMap on cypress build caps [SDK-7285]#1180kamal-kaur04 wants to merge 3 commits into
Conversation
… caps [SDK-7285] Cypress is the only SDK line that stamps no TestHub capability at all: 0 of 16,629 sessions carry one across all 32 CLI versions. That leaves it with a single route into Test Reporting & Analytics -- the BTCER session_hashed_id fallback -- and that misses ~32.5% of its sessions, which are then counted against ingestion stability with no build to attach to (2,917 sessions / 0.5882pp in the measured cohort, across 76 accounts). caps() already runs after build start, so the uuid and the product map are both in hand; getProductMap() is reused rather than reimplemented, and evaluating it here reports the products still enabled after the accessibility/observability gates rather than the ones requested. Both keys are written unconditionally. The metric treats an absent key and an empty string identically, so the empty string costs nothing and preserves the distinction between a cap writer that ran with no build to name and no writer running at all -- which for cypress is currently unobservable. Verified end-to-end on production: the build-create POST carries testhubBuildUuid=xcsouiygyettrxqdxyxmi18qnkyb6alfxqnj8x4z, matching the TestHub build the run created. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited), Workspace UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
A1.1b verdict: CONFIRMED. This PR is inert until the automate rails layer maps the two keys through.The probe session's row landed 34 minutes after the run ( The stored {"devices": ["Windows 11-chrome136"], "testSuite": "dd42ed9e0572e7f79410495f3a01fefa513808a8",
"npmPackageSuite": null, "cacheDependencies": 0,
"customBuildName": "SDK-7285-A1-e2e-20260909-133652", "local": 0, "localIdentifier": null,
"localVersion": null, "callbackURL": null, "projectNotifyURL": null,
"local_mode_inferred": "", "local_inferred": 0, "sync_inferred": 0, "geolocation": null,
"build_artifacts": 0, "video_config": {"video": false}, "spec_timeout": 1800000,
"interactiveDebugging": 1, "project": "TRA-A1-cypress-caps-instrumentation",
"node_version": "20.16.0", "network_logs": "false", "framework": "cypress", "headless": false}Same run, same build, minutes apart:
So this is no longer an inference from the caps corpus — it is demonstrated on a single controlled run. The CLI side is correct and complete; the keys are dropped downstream. Note what the stored hash does contain, which shows the mechanism rather than just the outcome: What I'd suggest for this PRThe diff is correct and I'd argue it should still land — it is a prerequisite, it is inert rather than risky, and shipping it early means the rails change has something to receive. But it should not be released with a customer-facing note claiming attribution works, because it will not until the rails leg is in. Two options, reviewer's call:
I've left it as draft and have not marked it ready. Incidental finding, worth knowing if you ever time-bound a query on this table
|
…s no build [SDK-7285] The product map is stamped twice with different meanings: on the build-start request it is INTENT (what the run asked for), and on the session caps it is OUTCOME (what is actually live). Only the first was true. extractDataFromResponse already clears BROWSERSTACK_TEST_OBSERVABILITY when a 2xx comes back carrying observability.success=false, but launchBuild's catch -- a non-2xx or a network error, so the whole build-start-failed population -- only logged. The flag stayed true, so the session reported observability enabled with no build behind it. Verified on production against an injected collector 400: buildProductMap.observability was true before this change and is false after, same config, same fault. Routing that catch through handleErrorForObservability fixes it in one place and makes the failure paths consistent. That funnel writes the string "null" into BROWSERSTACK_TESTHUB_UUID, which the caps must not stamp as if it were a real uuid -- so the two changes land together. An explicit testObservability: false in config already resolved correctly through setTestObservabilityFlags; that needed no change and is now covered by a test so it cannot regress silently. Also: logBuildError treated only undefined as "no error object", so a null error logged the string "null" instead of the intended message. Affected the accessibility no-response path too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second commit —
|
| BEFORE (unfixed) | AFTER | |
|---|---|---|
caps buildProductMap.observability |
true ❌ |
false ✅ |
BROWSERSTACK_TEST_OBSERVABILITY |
"true" |
"false" |
BROWSERSTACK_TESTHUB_UUID |
never set | "null" (sentinel, scrubbed from caps) |
BS_TESTOPS_BUILD_COMPLETED |
never set | "false" |
caps testhubBuildUuid |
"" |
"" |
| user's tests | 1 passed, exit 0 | 1 passed, exit 0 |
| Run | Automate build | Session |
|---|---|---|
Case (a), testObservability: false |
0a5a64bd53c8abaec11fd24d0bf922a73380041f |
— (see note) |
| Case (b), after | 916337360d759acd17cf7066d965d9463cf22ceb |
e435ffd0c34aea77ee2aeebe6650f1d2b3bca2c8 |
| Case (b), before (baseline) | 066f1b9fce74a5d2d69f4c985a22d392594511a2 |
8a1ecf09b0bbfb6ca3631833a8be9a9e8b69747f |
Case (a) on the wire: buildProductMap.observability = false, testhubBuildUuid = "", build create 201.
Graceful degradation held — both failed-build-start runs finished exit 0 with the customer's test passing. The injected error message still surfaced to the user through logBuildError.
Unit tests — and they demonstrably detect the defect
12 new tests (test/unit/bin/testhub/utils.js is new). Full suite on the rebased tree: 723 passing / 16 failing, those 16 being the same pre-existing master failures.
More usefully, 4 of the new tests fail against the unfixed source and pass against the fixed one — so they are defect detectors, not decoration:
1) turns observability off in the product map when build start fails
2) does not report observability as still enabled to shouldProcessEventForTesthub
3) treats a null error the same as a missing one
4) flips observability to false and drops the null sentinel when build start failed
The case-(a) test passes both before and after — deliberately, since that behaviour already worked and the test exists purely as a regression guard.
One thing for the reviewer to weigh
Clearing BROWSERSTACK_TEST_OBSERVABILITY on a failed build start also affects other readers of isTestObservabilitySession(), all of which I believe become more correct: printBuildLink already suppresses the link when BS_TESTOPS_BUILD_HASHED_ID == "null", stopBuildUpstream already gates on BS_TESTOPS_BUILD_COMPLETED === "true", and getLocalSessionReporter additionally requires BS_TESTOPS_JWT. The one visible difference is that utils.js:1281 now takes its else branch (process.exit(exitCode)) instead of calling printBuildLink — same terminal behaviour, since that function's only remaining act on this path was to exit with the same code. Worth a second pair of eyes.
Also noticed, not fixed here
Disabling observability can break a project whose cypress.config.js imports the BrowserStack plugin: browserstack-cypress-cli is added to npm_dependencies only as a side effect of observability succeeding (setTestObservabilityVariables → setBrowserstackCypressCliDependency). Case (a) hit this — 204 No specs ran — until the dependency was declared explicitly. Pre-existing, out of scope, deserves its own ticket.
What is this about?
Cypress is the only SDK line that stamps no TestHub capability at all — measured, 0 of 16,629 sessions carry one, across all 32 CLI versions present including the newest.
bin/has zero references totesthubBuildUuid,buildProductMapor abrowserstackSDKcap; the build id goes to the in-browser reporter instead.That leaves cypress with a single route into Test Reporting & Analytics — the BTCER
session_hashed_idfallback — where every other SDK line has two. That fallback misses ~32.5% of cypress sessions, and those sessions are then counted against ingestion stability with no build to attach to.Population: 2,917 orphaned sessions in the measured cohort · 0.5882 pp of instability · 76 accounts / 56 groups. Cypress is the only diffuse line in that bucket — top group 10.6%, 24 groups to reach 90%, where every other line is 83–100% a single customer. So this is a product gap affecting many customers a little, not a support case.
This PR adds the missing surface. It does not on its own move the metric — see Dependent PRs / release order.
Related Jira task/s
Dependent PRs / release order
capabilitieshashCAST(NULL AS STRING)zombiesl4_metrics.yml:2081-2087Why A1.1b exists. The
capabilitiescolumn the metric reads onautomate.cypress_test_sessions_partitionedis not the payload the CLI sends — it is rebuilt server-side from an explicit key list. Measured over 3,782 cypress sessions (BQjob_5VlbncWZuByJPM9vetoSvqNYMkmK):capabilitiesrun_settings(sent on every build)cypress_cli_user_agent(sent on every build)parallelstest_suitetestSuitelocal_mode_inferredThree keys sent on every build never arrive; one is renamed while another is preserved verbatim. Mixed casing plus outright drops rules out a blanket transform.
Automation cases to add
None. The change adds two keys to an outbound payload; there is no user-visible behaviour to assert from a wrapper test, and the value is only observable in BigQuery. Covered by unit tests here + the end-to-end wire capture below.
Code changes to check
bin/helpers/capabilityHelper.js— the two new keys, and the newrequireof../testhub/utilstest/unit/bin/helpers/capabilityHelper.js— newcontext("testhub build attribution")The change
Four things worth a reviewer's attention, all deliberate:
runs.js:137alreadyawaitsTestHubHandler.launchBuild, which setsBROWSERSTACK_TESTHUB_UUIDattesthub/utils.js:236, beforecapabilityHelper.validate→build.createBuild→caps(). Confirmed on the wire below.getProductMap()is reused, not reimplemented (bin/testhub/utils.js:38) — same five-product shape the other SDK lines stamp. Calling it at caps-build time is deliberate: it reports the products still enabled after the accessibility/observability gates atruns.js:118-137, rather than the ones originally requested.bin/helpers/utils.js:30already top-level-requires../testhub/utils, andcapabilityHelperalready requires./utils, so the module was in the graph.getProductMapis reached through the module object (late-bound) rather than destructured at load time, so a partially-initialised export cannot bite.|| ""). The metric treats an absent key and''identically, so the empty string costs nothing — and it preserves the distinction between a cap writer ran and had nothing to name and no cap writer ran at all, which for cypress is currently unobservable anywhere. Key names deliberately match the other five SDK lines so the A1.2 branch can reuse their existingREGEXP_EXTRACTverbatim.Second commit — the map must not lie about observability
buildProductMapis stamped twice with two different meanings: on the build-start request it is intent, on the session caps it is outcome. Only the first was honest.extractDataFromResponsealready clearsBROWSERSTACK_TEST_OBSERVABILITYfor a 2xx carryingobservability.success = false, butlaunchBuild'scatch— every non-2xx and every network error — only logged, leaving the flagtrue. The session then advertised observability with no build behind it. The collector rejects 7,442 × 400 + 442 × 403 build-starts per week, so this was the common path, not an edge case.Three coupled changes:
handleErrorForObservabilityalso clears the flag (one funnel, all failure paths agree);launchBuild's catch routes through it, before theerror.success === falsebranch so network errors are covered too; andcaps()no longer stamps the"null"sentinel as a uuid — which is required because of the second change, since that is what now causes the sentinel to be written on this path.An explicit
testObservability: falsein config already resolved correctly and was left alone; it is covered by a test purely as a regression guard.Verification
Unit — 2 new tests; whole-suite comparison against a stashed baseline, so "no regressions" is measured rather than asserted:
8731a23)Same 16 failures either way — all pre-existing on
master(runs×7,fileHelpers×2,readCypressConfigUtil×3,syncSpecsLogs×1,utils › getVideoConfig×3). Heads-up that master's unit suite does not currently pass clean.End-to-end on production infra — patched CLI served from source,
--cli-debug --sync, 1 spec / 2 tests, Chrome 136 / Windows 11.Total tests: 1, passed: 1, failed: 0, exit 0.xcsouiygyettrxqdxyxmi18qnkyb6alfxqnj8x4zb1caa6889d0ce4122dd9c601374d9462e01f389becbded54b9f90a9a4dc9ebffdac03db3ae1d6e21CLI debug, showing the value was in hand before caps were built:
On the wire — captured by wrapping
axios.postfrom outside the repo, so this is the request as sent rather than the CLI's own account of it:The uuid on the caps payload is byte-identical to the TestHub build the same run created, and the hub accepted the payload with both previously-unknown keys present (201) — which settles the open "confirm the hub tolerates an unknown cap key before rollout" question in the affirmative.
What is NOT verified here: whether the keys survive into
automate.cypress_test_sessions_partitioned.capabilities. That is leg A1.1b and is a property of the automate rails layer, not of this diff. The probe session's row had not landed at the time of writing; I will post the result as a comment either way. Note the metric excludesgroup_id IN (2, 1224686)and this run is group 2, so the probe is plumbing-only and never enters the stability numbers.Release
Version bump:
Release notes type:
Release notes (customer-facing):
Release notes (internal):
bin/helpers/capabilityHelper.js:caps()now stampstesthubBuildUuid(fromBROWSERSTACK_TESTHUB_UUID, set byTestHubHandler.launchBuildbeforecaps()runs) andbuildProductMap(from the existingtesthub/utils.jsgetProductMap()) on the build-creation payload. Cypress previously stamped no TestHub capability on any session (0 of 16,629 measured), leaving the BTCERsession_hashed_idfallback as its only route into TRA; that fallback misses ~32.5%, worth 2,917 orphaned sessions / 0.5882 pp across 76 accounts. Both keys are written unconditionally so an empty uuid is distinguishable from an absent writer.test/unit/bin/helpers/capabilityHelper.js→context("testhub build attribution").bin/testhub/utils.js+bin/testhub/testhubHandler.js:handleErrorForObservabilitynow also clearsBROWSERSTACK_TEST_OBSERVABILITY, andlaunchBuild's catch routes through it rather than only logging — so a non-2xx or network failure on build start no longer leavesbuildProductMap.observabilityreportingtruewith no build behind it (verified against an injected collector 400:truebefore,falseafter).caps()additionally guards against stamping the"null"uuid sentinel that funnel writes. AlsologBuildErrortreated onlyundefinedas "no error object", so anullerror logged the string"null"— this affected the accessibility no-response path as well.capabilitieshash (that column is rebuilt server-side from an explicit key list — three keys the CLI sends on every build appear 0/3,782 times), and until thezombiescypress branch extractstesthubBuildUuidinstead of hardcodingCAST(NULL AS STRING).Checklist
Reviewer notes
The one judgement call is point 4 above — writing
""rather than omitting the key when build start produced no uuid. The metric cannot tell the two apart today, so this is free on the measurement side; the reason to prefer the empty string is diagnostic, and it is the only thing that would let us separate build start failed from TestHub was never requested on a cypress session. Happy to guard it instead if you would rather the key never appear empty.🤖 Generated with Claude Code