Repository navigation
ResourceManager Cardinality: drop resource_id from usage records - #2472
Conversation
…e records Usage record resource ids are per-execution, so labeling the gauge with them creates an unbounded series set per node. On CRE staging zone-a this grew by thousands of series per hour until the OTLP export exceeded the collector's 16 MiB limit and every platform_engine_* metric was dropped. Snapshots keep the label: their resource id is the workflow id, which is bounded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
👋 DylanTinianov, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
📊 API Diff Results
|
There was a problem hiding this comment.
🟡 Changes recommended
Label removal also affects durable-resource deltas, incorrectly merging previously distinct resource series.
1 open finding
What changed in this PR
Reduces utilization metric cardinality by removing per-execution resource IDs from usage measurements while retaining snapshot IDs.
Changes:
- Adds conditional
resource_idlabeling to the utilization gauge. - Adds tests for usage-series consolidation and snapshot labels.
| File | Description |
|---|---|
| pkg/resourcemanager/utilization_gauge_test.go | Tests usage and snapshot labeling. |
| pkg/resourcemanager/resourcemanager.go | Controls resource ID labels when recording utilization. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
DylanTinianov
changed the title
Only usage records have per-execution ids. Deltas address durable resources and must share a series with the matching snapshot. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 19c347c | Previous: b22d56c | Ratio |
|---|---|---|---|
BenchmarkKeystore_Sign/nop/out-of-process |
57786 ns/op |
28372 ns/op |
2.04 |
BenchmarkKeystore_Sign/hex/out-of-process |
57004 ns/op |
26722 ns/op |
2.13 |
BenchmarkKeystore_Sign/ed25519/out-of-process |
95744 ns/op |
47119 ns/op |
2.03 |
This comment was automatically generated by workflow using github-action-benchmark.
The id can be per-execution, so it stays on the durable MeterRecord and MeterSnapshot events only. No caller option. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
What
resource_manager_utilizationno longer carries aresource_idlabel, for any record type. The id stays on the durableMeterRecord/MeterSnapshotevents.Why
Usage record resource ids are per execution (
WorkflowUsageResourceID(workflowID, executionID)), so the gauge gained a new series for every execution on every node. That series set is unbounded and eventually makes the node's metric export payload too large for the collector to accept, at which point every metric from that process is dropped.Dropping the label outright, rather than per record type, removes the option to reintroduce the cardinality later.
Testing
TestUtilizationGauge_NoResourceIDLabelreads the gauge through an OTel manual reader: usage, delta and snapshot records with distinct resource ids collapse to one series perresource_typewith noresource_id, and the emitted records still carry every id.🤖 Generated with Claude Code