mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 08:03:20 +00:00
fix(relay): cross-multiply the skew bars so a zero placement share still fires
`hint_share / placement_share` is undefined in the hour that matters most. When the director placed nobody in the region, MQL returns no rows for either 0/0 or x/0, so the series disappears before the gap and volume clauses run and the alert stays silent. That hour is not hypothetical: it is every desktop asking for a region while the director puts nobody there, which is what a drained, fenced, or full region looks like, and it is the most extreme skew the alert can see. The condition is now cross-multiplied, `hint_share > 2 * placement_share`, which is well defined at zero. Both forms were run read-only against production surrogates chosen so the placement denominator is exactly zero: the ratio form returned no rows, the cross-multiplied form returned the series with the condition true on every point. A second surrogate pass with a tiny hint share returned the series with the condition false, so the gap clause still suppresses the healthy shape rather than the query silently matching everything. The flat field names are no longer derived on either side. Terraform title cased each dash-separated part and the emitter upper cased each part's first character, so the ratchet had to pin two source expressions by regex, which a reformat would break and which never compared the actual rendered names. Both sides now declare a literal map, relay-contract's RELAY_REGION_METRIC_SEGMENTS and Terraform's relay_region_field_segments, and the test compares the two declarations against each other and against the expected names. `satisfies Record<RelayRegion, string>` makes a region added without a segment a compile error rather than a silent gap in the alert's denominators. Both ratchets were checked by mutation: a wrong Terraform segment, a contract region with no Terraform entry, and a revert to the ratio form each fail the node test, and the new region fails the contract build.
This commit is contained in:
@@ -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', () => {
|
||||
|
||||
@@ -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<string, number>
|
||||
): Record<string, number> {
|
||||
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
|
||||
])
|
||||
)
|
||||
|
||||
@@ -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 `<prefix><Segment>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}<Segment>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'),
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
@@ -8,6 +8,15 @@ export type RelayRegion = z.infer<typeof RelayRegionSchema>
|
||||
|
||||
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<RelayRegion, string>
|
||||
|
||||
const RelayProbeOriginSchema = z.string().url().max(2_048).refine(isCanonicalHttpsOrigin)
|
||||
|
||||
export const RelayRegionCatalogResponseSchema = z
|
||||
|
||||
Reference in New Issue
Block a user