diff --git a/cloud/apps/relay/src/relay-observability.test.ts b/cloud/apps/relay/src/relay-observability.test.ts index edc1f7c71f1..22802b7f8aa 100644 --- a/cloud/apps/relay/src/relay-observability.test.ts +++ b/cloud/apps/relay/src/relay-observability.test.ts @@ -1,4 +1,4 @@ -import { RELAY_REGIONS } from '@orca-cloud/relay-contract' +import { RELAY_REGION_METRIC_SEGMENTS, RELAY_REGIONS } from '@orca-cloud/relay-contract' import { describe, expect, it, vi } from 'vitest' import type { RelayDatabase } from './database.js' import { observeRelayDatabase } from './observed-relay-database.js' @@ -133,14 +133,11 @@ describe('relay observability', () => { }) // A region added to the contract has to reach the flat keys, or the skew alert's // denominator silently misses it. - for (const region of RELAY_REGIONS) { - const segment = region - .split('-') - .map((part) => part.charAt(0).toUpperCase() + part.slice(1)) - .join('') + for (const segment of Object.values(RELAY_REGION_METRIC_SEGMENTS)) { expect(entries[0]).toHaveProperty(`requestedRegion${segment}Delta`) expect(entries[0]).toHaveProperty(`selectedRegion${segment}Delta`) } + expect(Object.keys(RELAY_REGION_METRIC_SEGMENTS).sort()).toEqual([...RELAY_REGIONS].sort()) }) it('emits bounded aggregate runtime signals without identities or credentials', () => { diff --git a/cloud/apps/relay/src/relay-observability.ts b/cloud/apps/relay/src/relay-observability.ts index ac574fdcb89..9f937afdb6e 100644 --- a/cloud/apps/relay/src/relay-observability.ts +++ b/cloud/apps/relay/src/relay-observability.ts @@ -1,5 +1,5 @@ import { monitorEventLoopDelay, performance } from 'node:perf_hooks' -import { RELAY_REGIONS, type RelayRegion } from '@orca-cloud/relay-contract' +import { RELAY_REGION_METRIC_SEGMENTS, type RelayRegion } from '@orca-cloud/relay-contract' import type { ControlRenewalOutcome } from './assignment-store.js' import type { CellInventoryHoldCounts } from './cell-inventory-hold-samples.js' import type { PostgresPoolPressureCounts } from './postgres-pool-pressure.js' @@ -419,20 +419,13 @@ export class RelayObservability implements RelayRuntimeObserver { // A log-based metric cannot reach `requestedRegionsDelta."asia-east2"` without a quoted field // path, and an absent key would drop a series out of the inner join the region-skew alert does. // The maps stay authoritative and keep carrying anything outside the catalog, such as `unhinted`. -function regionFieldSegment(region: RelayRegion): string { - return region - .split('-') - .map((part) => part.charAt(0).toUpperCase() + part.slice(1)) - .join('') -} - function regionCounterFields( prefix: 'requestedRegion' | 'selectedRegion', counts: Record ): Record { return Object.fromEntries( - RELAY_REGIONS.map((region) => [ - `${prefix}${regionFieldSegment(region)}Delta`, + Object.entries(RELAY_REGION_METRIC_SEGMENTS).map(([region, segment]) => [ + `${prefix}${segment}Delta`, counts[region] ?? 0 ]) ) diff --git a/cloud/dev/scripts/relay-region-hint-metrics.test.mjs b/cloud/dev/scripts/relay-region-hint-metrics.test.mjs index 3f891009099..8f331efce18 100644 --- a/cloud/dev/scripts/relay-region-hint-metrics.test.mjs +++ b/cloud/dev/scripts/relay-region-hint-metrics.test.mjs @@ -19,7 +19,6 @@ const contractRegions = (() => { })() const terraform = read('../../infra/terraform/relay-observability.tf') -const emitter = read('../../apps/relay/src/relay-observability.ts') const terraformRegions = (() => { const literal = /relay_region_keys = \[([^\]]*)\]/.exec(terraform) @@ -27,26 +26,35 @@ const terraformRegions = (() => { return [...literal[1].matchAll(/"([^"]+)"/g)].map((match) => match[1]) })() +// Both sides now spell the field-name segments out, so the test compares the two declared maps +// rather than two source expressions. Reformatting either file cannot break this, and a literal +// expected value below still catches an identical wrong edit made to both. +const declaredSegments = (source, open, close) => { + const body = source.slice(source.indexOf(open) + open.length, source.indexOf(close, source.indexOf(open))) + return Object.fromEntries( + [...body.matchAll(/'?"?([a-z0-9-]+)'?"?\s*[:=]\s*'?"?([A-Za-z0-9]+)'?"?/g)].map((match) => [ + match[1], + match[2] + ]) + ) +} + +const terraformSegments = declaredSegments(terraform, 'relay_region_field_segments = {', '}') +const contractSegments = declaredSegments( + read('../../packages/relay-contract/src/relay-regions.ts'), + 'RELAY_REGION_METRIC_SEGMENTS = {', + '}' +) + test('terraform covers exactly the regions the contract can hint or select', () => { assert.deepEqual([...terraformRegions].sort(), [...contractRegions].sort()) }) -test('terraform and the emitter derive the same flat field names', () => { - // Both build `Delta` from the hyphenated region id: Terraform title-cases each - // dash-separated part, the emitter upper-cases each part's first character. Same result, two - // languages, so the rules are pinned rather than the rendered names. - assert.match(collapse(terraform), /join\("", \[for part in split\("-", key\) : title\(part\)\]\)/) - assert.match( - collapse(emitter), - /\.split\('-'\) \.map\(\(part\) => part\.charAt\(0\)\.toUpperCase\(\) \+ part\.slice\(1\)\)/ - ) - for (const prefix of ['requestedRegion', 'selectedRegion']) { - assert.ok( - terraform.includes(`${prefix}\${local.relay_region_field_segments[key]}Delta`), - `terraform does not build ${prefix}Delta` - ) - assert.ok(emitter.includes(`'${prefix}'`), `the emitter does not publish ${prefix} counters`) - } +test('terraform and the contract declare the same flat field segments', () => { + assert.deepEqual(terraformSegments, contractSegments) + // Pinned literally so the same wrong edit applied to both sides still fails. + assert.deepEqual(terraformSegments, { 'us-central1': 'UsCentral1', 'asia-east2': 'AsiaEast2' }) + assert.deepEqual(Object.keys(terraformSegments).sort(), [...contractRegions].sort()) }) test('the skew query compares a catalogued region against itself', () => { @@ -58,6 +66,16 @@ test('the skew query compares a catalogued region against itself', () => { assert.ok(columns.includes(hint[1]), `${hint[1]} is not one of ${columns.join(', ')}`) }) +test('the skew condition never divides by the placement share', () => { + // A zero-placement hour is the worst skew there is; MQL drops the row on x/0, so the ratio form + // silences exactly the case the alert exists for. + assert.ok( + !/hint_share \/ placement_share/.test(terraform), + 'cross-multiply instead: hint_share > 2 * placement_share' + ) + assert.match(collapse(terraform), /condition hint_share > 2 \* placement_share/) +}) + test('the unhinted bucket stays out of the skew denominators', () => { assert.ok( !terraformRegions.includes('unhinted'), diff --git a/cloud/docs/relay-incident-monitor.md b/cloud/docs/relay-incident-monitor.md index 8d0b93c03e5..696efb85296 100644 --- a/cloud/docs/relay-incident-monitor.md +++ b/cloud/docs/relay-incident-monitor.md @@ -161,7 +161,11 @@ Threshold basis: the broken state and outside a healthy one. `unhinted` requests are excluded from the denominator: they were 27% of all requests, so a client change that always sends a hint would move the number with no behaviour - change at all. + change at all. The two bars are cross-multiplied rather than divided. An + hour that placed nobody in the region is the most extreme skew there is, + and it happens whenever the region is drained, fenced, or at capacity, but + dividing by that zero placement share makes MQL drop the row and lose the + series before any other clause runs. Expect the skew alert to stay lit after a client fix until the mis-homed backlog is rehomed. Sticky assignment never re-consults the hint, so a @@ -185,7 +189,10 @@ every interval, not the nested region maps: a log-based metric would need a quoted field path to reach a hyphenated map key, and an absent key would drop a series out of the inner join. The region list lives in Terraform as `relay_region_keys` and is pinned to relay-contract's `RELAY_REGIONS` by -`dev/scripts/relay-region-hint-metrics.test.mjs`. +`dev/scripts/relay-region-hint-metrics.test.mjs`. Both sides spell the field +name segments out as literal maps rather than deriving them, so the same test +compares the two declarations directly. Adding a region to the contract +without its segment is a compile error in relay-contract, not a silent gap. ## Implementation log diff --git a/cloud/infra/terraform/relay-observability.tf b/cloud/infra/terraform/relay-observability.tf index 83fbb9c960e..4c6722d3532 100644 --- a/cloud/infra/terraform/relay-observability.tf +++ b/cloud/infra/terraform/relay-observability.tf @@ -100,10 +100,12 @@ locals { relay_region_keys = ["us-central1", "asia-east2"] # Flat emitter fields, not the nested `requestedRegionsDelta` map: a log-based metric would need # a quoted field path to reach a hyphenated map key, and the relay publishes these as zeros in - # every interval so no series can drop out of the alert's inner join. + # every interval so no series can drop out of the alert's inner join. Spelled out rather than + # derived, so this literal and relay-contract's RELAY_REGION_METRIC_SEGMENTS can be compared + # directly; reformatting either side cannot break the check and neither can drift alone. relay_region_field_segments = { - for key in local.relay_region_keys : - key => join("", [for part in split("-", key) : title(part)]) + "us-central1" = "UsCentral1" + "asia-east2" = "AsiaEast2" } relay_region_columns = { for key in local.relay_region_keys : key => replace(key, "-", "_") } relay_region_share_metrics = merge( @@ -155,12 +157,14 @@ locals { " placement_share: sel_asia_east2 / (${local.relay_region_selected_total}),", " hinted_requests: ${local.relay_region_hinted_total}", " ]", - "| value [", - " divergence: hint_share / placement_share,", - " gap: hint_share - placement_share,", - " hinted_requests: hinted_requests", - " ]", - "| condition divergence > 2 '1' && gap > 0.15 '1' && hinted_requests > 500 '1'" + # Cross-multiplied, never a plain ratio of the two shares: an hour that placed nobody in the + # region makes that ratio 0/0 or x/0, and MQL drops the row instead of yielding a number, so + # the whole series vanishes before the other clauses run. That hour is the worst skew there + # is - every desktop asking for a region the director is putting nobody in - and it happens + # whenever the region is drained, fenced, or at capacity. Both forms were run read-only + # against production surrogates with a zero denominator: the ratio returned no rows, this + # returned the series with the condition true. + "| condition hint_share > 2 * placement_share && hint_share - placement_share > 0.15 '1' && hinted_requests > 500 '1'" ] )) relay_custom_alerts = { diff --git a/cloud/packages/relay-contract/src/relay-regions.ts b/cloud/packages/relay-contract/src/relay-regions.ts index 38ac36cd738..6b8837829df 100644 --- a/cloud/packages/relay-contract/src/relay-regions.ts +++ b/cloud/packages/relay-contract/src/relay-regions.ts @@ -8,6 +8,15 @@ export type RelayRegion = z.infer export const RELAY_DEFAULT_REGION: RelayRegion = 'us-central1' +// Field-name segment for the flat per-region runtime counters, spelled out rather than derived so +// the Terraform side can hold the same literal and a test can compare the two. `satisfies` makes a +// new region a compile error here, which is the point: a region with no segment would silently +// drop out of the region-skew alert's denominators. +export const RELAY_REGION_METRIC_SEGMENTS = { + 'us-central1': 'UsCentral1', + 'asia-east2': 'AsiaEast2' +} as const satisfies Record + const RelayProbeOriginSchema = z.string().url().max(2_048).refine(isCanonicalHttpsOrigin) export const RelayRegionCatalogResponseSchema = z