fix(batch): more high cardinality metric attribute fixes#2846
Conversation
|
WalkthroughThis pull request introduces OpenTelemetry metrics cardinality guidelines and refactors batch-queue metrics. A new guideline document is added at .cursor/rules/otel-metrics.mdc that defines cardinality best practices, provides do's and don'ts, and includes TypeScript examples. Concurrently, metrics in internal-packages/run-engine/src/batch-queue/index.ts are updated to replace environment ID-based attribute keys with environment_type keys derived from meta.environmentType or options.environmentType, applied consistently across enqueued, processed, failed, and duration-related metrics. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Review: fix(batch): more high cardinality metric attribute fixesSummaryThis PR correctly addresses high-cardinality metric attribute issues in the BatchQueue by replacing unbounded attributes ( ✅ Code Quality & Best PracticesPositives:
Suggestions:
🐛 Potential Issues
📊 Performance ConsiderationsPositive impacts:
No negative performance impacts - the changes only modify attribute values, not the metric recording logic itself. 🔒 Security ConcernsNo security concerns with this PR. The changes:
🧪 Test CoverageThe PR doesn't include tests, but this is reasonable because:
📝 Minor Observations
✅ RecommendationLGTM - This PR correctly fixes the high-cardinality issue. The The documentation in |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.cursor/rules/otel-metrics.mdc:
- Around line 1-53: The referenced schedule engine example violates the
guideline by attaching a high-cardinality UUID attribute; change the metric call
that uses devEnvironmentCheckCounter.add(1, { environment_id:
instance.environment.id }) to pass a low-cardinality attribute instead (e.g.,
environment_type: instance.environment.type or another bounded enum) so the
metric uses environment_type rather than environment_id, and update any related
tests or telemetry consumers expecting the old attribute name; alternatively, if
you prefer to keep the schedule engine unchanged, update the guideline reference
text to point to a corrected example that uses environment_type.
In @internal-packages/run-engine/src/batch-queue/index.ts:
- Around line 289-292: The itemsFailedCounter metric currently includes a
free-form string attribute errorCode (set from the ProcessBatchItemCallback
result at usages around itemsFailedCounter at lines ~813 and ~853), which can
cause high cardinality; either stop including errorCode in the metric attributes
(remove it from itemsFailedCounter.add calls and rely on structured logs for the
raw error) or normalize it to a bounded enum/union of known error categories
(map arbitrary callback error strings to a small set of stable codes like
"validation", "transient", "permanent", "unknown" before adding to
itemsFailedCounter). Update the code paths that increment itemsFailedCounter
(references: itemsFailedCounter, ProcessBatchItemCallback result handling) so
they no longer pass unbounded errorCode directly — instead pass one of the
limited enum values or omit the attribute entirely and emit the full error to
logs.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.cursor/rules/otel-metrics.mdcinternal-packages/run-engine/src/batch-queue/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
internal-packages/run-engine/src/batch-queue/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
internal-packages/run-engine/src/batch-queue/index.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
internal-packages/run-engine/src/batch-queue/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (1)
internal-packages/run-engine/src/batch-queue/index.ts (1)
703-736: Comment improvements enhance code clarity.The updated comments accurately reflect:
- Queue time calculation logic (line 703-704)
- The requirement to fetch
metafor theenvironment_typeattribute (line 735-736)This makes the code's intent clearer, especially the dependency on
meta.environmentTypefor the metric.
No description provided.