feat(metrics): promote aggregate_key and phase so a wedged fence is nameable (BLO-32163) - #1695
feat(metrics): promote aggregate_key and phase so a wedged fence is nameable (BLO-32163)#1695allyblockcast[bot] wants to merge 1 commit into
Conversation
…ameable (BLO-32163) PEN-2799 landed the path from `ctx.metrics.write` to Prometheus, but a plugin tag only becomes a label when it appears in BOTH the platform allow-list and the emitting plugin's manifest `metricLabels`. `alertmanager.aggregate. fence_blocked` tags its writes with `aggregate_key` and `phase`, and neither key was on either list — so the series publishes with the wedged aggregate anonymous, and an alert on it could say only that *a* fence is stuck. Both halves are added here, and both are pinned by tests. The gate is two-sided and fails silently: dropping either key leaves the metric publishing, the rule evaluating, and the graph gap-free while the page quietly stops naming which aggregate is wedged. There is no error to notice, so review alone is not a sufficient guard. The cardinality bound is argued at the allow-list rather than assumed: `phase` is a closed four-value vocabulary, and `aggregate_key` is `alertname x dedupe-domain`, which is bounded by the alert-rule registry -- not by alert instances, which is the axis that would actually be unbounded. Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d50f346
The change itself is correct, minimal, and sequenced the right way round — landing the label
allow-list before the emitter is the safe order, since the reverse would silently drop the tags.
One finding, and it is about the PR's stated premise rather than its code.
Critical Issues (0)
Important Issues (1)
-
[native-codex / gstack-review]
server/src/__tests__/plugin-metric-exposition.test.ts:645— The
PR body asserts as present-tense fact that "alertmanager.aggregate.fence_blockedtags its writes
withaggregate_keyandphase" and that the series "publishes with the wedged aggregate
anonymous". At this head, that metric does not exist. Enumerated everymetrics.writecall
site in the plugin (21 calls, 19 distinct metric names, all inwebhook-handler.ts): none is
fence_blocked, and none passesaggregate_keyorphaseas a tag — the aggregate metrics
(alertmanager.aggregate.joined,.rebound) pass only{alertname, severity}. The
aggregate_key/phasepair that does appear inwebhook-handler.ts:253is a
PluginFencingPreconditionSQL match clause, not a metric tag.The emitter is added by #1677 (open, not
merged), which introducesalertmanager.aggregate.fence_blockedwith exactly these two tags. This
PR never mentions that dependency, and the "Scope / what this does NOT do" section defers only the
alert rule — describing the emit writes in the present tense ("the emit writes hold-age
seconds"), which reads as though the emitter is already shipped.Why this is worth flagging rather than shrugging at: the PR's own rationale is that this gate
"fails silently — no error, no missing data, the page just quietly stops naming which
aggregate is wedged, so review alone is not a sufficient guard." Both new tests pass green while
supplying the metric name and tags themselves (declaredLabels: MANIFEST_METRIC_LABELSat
:650, tags at:646-651), so they would pass identically if #1677 never landed. CI green here
therefore does not indicate the fence is nameable — reproducing the exact failure mode the PR
exists to prevent, one level up. The concrete risk is BLO-32163's acceptance criterion being
marked met on this merge alone, while the fence stays anonymous in production.- No code change needed — the sequencing is right. Correct the body to say the emitter is
introduced by #1677 and not yet merged, and state the merge dependency explicitly (#1695 first,
then #1677). If BLO-32163 has an AC tied to "a wedged fence is nameable from the page", note on
the issue that it is satisfied only once both land.
- No code change needed — the sequencing is right. Correct the body to say the emitter is
Suggestions (2)
-
[gstack-review]
server/src/services/metrics.ts:751—aggregate_keyis the only promotable
key whose value carries a fixed structural prefix (alert-aggregate:v1:[", 21 chars before any
distinguishing content). WithPLUGIN_METRIC_LABEL_VALUE_MAX_LENGTH = 128, two aggregates whose
alertname+dedupe-domainagree on the first ~104 characters truncate to the same label value —
and the doc at:2296-2300is explicit that "two values sharing that prefix collapse into one
series". That collapse defeats precisely the "name the wedged fence" goal, and it lands on the
identifying label rather than a descriptive one. It is observable via thevalue_truncateddrop
reason, so not silent — but consider asserting a length bound inaggregateKeyForAlert, or noting
the ceiling in the allow-list comment alongside the cardinality argument. -
[code / type-design]
server/src/services/metrics.ts:768— The allow-list is documented as
bounding "what any plugin may promote", but the cardinality argument foraggregate_keyis
derived from one plugin's specific key format. Unlike its neighbours (action,decision,
severity,phase), which are generic closed vocabularies,aggregate_keyreads generic while
its bound is alertmanager-specific — a third-party plugin could promote a per-instance
aggregate_keyand inherit the label. Worth a clause in the comment scoping the argument to the
emitting plugin, so the next reader does not treat the bound as generic.
Strengths
- Sequencing is right and non-obvious: labels before emitter is the only safe order here, since the
reverse silently drops tags with no error. - The two-sided gate is pinned on both sides with the package-boundary reasoning written down,
rather than pulling a plugin into the server's dependency graph for one assertion. - The platform half asserts on the rendered exposition (
:657-661), not internal bookkeeping —
so promoting the key while mangling the value still fails. The quote-escaping expectation
(\"for prom-client) is correct. vitest.config.tsinclude: ["src/**/*.test.ts"]does pick up the newsrc/-root test despite
every sibling living insrc/__tests__/— checked, not assumed.- Top-level
afterEach(__resetMetricsForTest)at:25means the new describe block starts clean, so
toHaveLength(1)is not order-dependent. - Cardinality is argued at the allow-list with the unbounded axis (alert instances) named and
excluded, rather than asserted.
Recommended Action
- No Critical issues — nothing blocks merge on code grounds.
- Correct the PR body's present-tense claim about the emitter and state the #1677 merge dependency
before this lands, so the issue is not closed on a half that cannot yet name a fence. - Consider the two Suggestions opportunistically.
Issue: https://paperclip.blockcast.net/BLO/issues/BLO-32163
What this changes
PEN-2799 (#1605, merged) built the path from
ctx.metrics.writeto Prometheus. This adds the two labels that make the alertmanager aggregatefence signal actionable on that path.
A plugin tag becomes a Prometheus label only when it appears in both:
PLUGIN_METRIC_PROMOTABLE_TAG_KEYS(server/src/services/metrics.ts), andmetricLabels.alertmanager.aggregate.fence_blockedtags its writes withaggregate_keyandphase, and neitherkey was on either list. The series therefore publishes with the wedged aggregate anonymous — an
alert built on it could say only that a fence is stuck, not which. Both halves are added here.
Why both halves are pinned by tests
The gate fails silently. Dropping either key leaves the metric publishing, the rule evaluating,
and the graph gap-free — the page just quietly stops naming which aggregate is wedged. There is no
error and no missing data, so review alone is not a sufficient guard.
The two halves cannot see each other at build time (the alertmanager plugin is deliberately not a
server dependency), so each side pins its own:
server/src/__tests__/plugin-metric-exposition.test.ts— platform half, asserted on the renderedexposition, so promoting the key but mangling the value still fails.
packages/plugins/paperclip-plugin-alertmanager/src/manifest-metric-labels.test.ts— manifest half.Cardinality
Argued at the allow-list rather than asserted:
phase— the fence lifecycle's phase column, a closed four-value vocabulary.aggregate_key—alert-aggregate:v1:["<alertname>",<dedupe-domain>], soalertname × dedupe-domain.alertnameis already admitted as bounded by the alert-rule registry, anddedupe-domainis arule-author opt-in that is null on every rule not setting it. Bounded by the rule registry, not
by alert instances — which is the axis that would actually be unbounded.
Verification
Scope / what this does NOT do
This lands the label half of BLO-32163. The alert rule is deliberately not here: the emit writes
hold-age seconds into
paperclip_plugin_metric_total, which is a Counter, so the series is acumulative sum of observed hold ages rather than a hold-age gauge. A threshold rule on it would page
on routine fence contention, violating the issue's third acceptance criterion. That finding and the
emit-shape change it requires are tracked on the issue.