Merged
Conversation
📝 WalkthroughWalkthroughThe PR refactors the metrics package to support custom labels in Prometheus metrics. It introduces a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/metrics/lib/prometheus/metrics/message-time/PrometheusMessageTimeMetric.ts (1)
30-36: Minor inconsistency: usingthis.metricParamsinstead of the method parameter.Line 35 accesses
this.metricParams.labelNameswhile the method receivesmetricParamsas a parameter. Although they reference the same object at runtime (due to base class behavior), using the parameter would be more consistent and make the method's data flow clearer.♻️ Suggested fix
labelNames: [ 'messageType', 'version', 'queue', 'result', - ...(this.metricParams.labelNames ?? []), + ...(metricParams.labelNames ?? []), ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/metrics/lib/prometheus/metrics/message-time/PrometheusMessageTimeMetric.ts` around lines 30 - 36, In the labelNames array inside PrometheusMessageTimeMetric (the method that builds the metric config), replace usage of this.metricParams.labelNames with the method parameter metricParams.labelNames to keep the data flow consistent; update the spread to ...(metricParams.labelNames ?? []) so the function consistently uses its incoming metricParams rather than the instance field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.spec.ts`:
- Around line 32-39: The helper mockCounterCalls hides regressions because it
fakes promClient.register.getSingleMetric and prevents createMetric() from ever
constructing a real client.Counter and exposing its labelNames; change the test
to use a real Counter on a fresh/cleared promClient.register or alternatively
spy on client.Counter's constructor so you can capture its constructor args
(labelNames) while still allowing the real Counter behavior; specifically update
mockCounterCalls to clear the registry (or stop mocking
register.getSingleMetric), create or spy-on client.Counter to capture labelNames
and preserve real inc() behavior, and assert against those captured labelNames
in the PrometheusMessageCounter tests.
In
`@packages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.ts`:
- Around line 7-11: The current PrometheusMetricCounterParams type allows
partial or invalid label sets because it types labelNames as Labels[]; change
the API to accept a readonly tuple for labelNames (e.g. labelNames: readonly
[...L]) and derive the union as type Labels = typeof labelNames[number] so
callers must pass a const tuple (const labelNames = [...] as const) to get
precise unions; update PrometheusMetricCounterParams/PrometheusMetricParams
usages to expect a readonly tuple and add a compile-time exclusion for reserved
labels (e.g. require Extract<Labels, Reserved> extends never) and/or enforce
exact coverage by validating the tuple-derived union matches the expected Labels
set.
In `@packages/metrics/README.md`:
- Around line 64-66: Add a language tag ("text") to the fenced code blocks that
contain the three formula snippets so markdownlint stops flagging them; locate
the triple-backtick blocks that wrap the formulas "value =
messageProcessingEndTimestamp - messageProcessingStartTimestamp", "value =
messageProcessingEndTimestamp - messageTimestamp", and "value =
messageProcessingStartTimestamp - messageTimestamp" and change their opening
fences from ``` to ```text, and do the same for the other two identical
occurrences mentioned (the blocks around those formulas at the other locations).
- Around line 165-170: The code reads metadata.processingResult.retryReason
without narrowing to a variant that actually contains that field; change
getLabelValuesForProcessedMessage to first narrow the union (e.g., const pr =
metadata.processingResult; if (pr.status === 'retryLater' && 'retryReason' in
pr) return { reason: pr.retryReason }; else return { reason: 'unknown' }) or
implement an explicit type-guard for the RetryLater variant before accessing
retryReason, or alternatively switch this metric to an error-based counter
instead of a reason label.
---
Nitpick comments:
In
`@packages/metrics/lib/prometheus/metrics/message-time/PrometheusMessageTimeMetric.ts`:
- Around line 30-36: In the labelNames array inside PrometheusMessageTimeMetric
(the method that builds the metric config), replace usage of
this.metricParams.labelNames with the method parameter metricParams.labelNames
to keep the data flow consistent; update the spread to
...(metricParams.labelNames ?? []) so the function consistently uses its
incoming metricParams rather than the instance field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c42800ce-c219-46a2-a64e-da18640a4e7a
📒 Files selected for processing (11)
packages/metrics/README.mdpackages/metrics/lib/index.tspackages/metrics/lib/prometheus/PrometheusMessageMetric.tspackages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageByStatusCounter.spec.tspackages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageByStatusCounter.tspackages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.spec.tspackages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.tspackages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageErrorCounter.spec.tspackages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageErrorCounter.tspackages/metrics/lib/prometheus/metrics/message-time/PrometheusMessageTimeMetric.spec.tspackages/metrics/lib/prometheus/metrics/message-time/PrometheusMessageTimeMetric.ts
packages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.spec.ts
Show resolved
Hide resolved
packages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.ts
Outdated
Show resolved
Hide resolved
kjamrog
approved these changes
Mar 16, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.ts (1)
44-48: Default implementation returns empty object regardless ofLabelsconstraint.When
Labelsis notnever, subclasses must overridegetLabelValuesForProcessedMessageto provide values for the registeredlabelNames. The default implementation returning{} as LabelValues<Labels>works because:
- When
Labelsisnever,LabelValues<never>accepts an empty object- When
Labelsis specified, the type system requireslabelNamesin params, signaling to developers they must provide valuesThis is a reasonable API design, but consider adding a JSDoc comment clarifying that subclasses must override this method when specifying custom
Labels.📝 Suggested documentation
+ /** + * Override this method to provide values for custom labels. + * Must be implemented when the class specifies a non-never Labels type. + */ protected getLabelValuesForProcessedMessage( _metadata: ProcessedMessageMetadata<MessagePayload>, ): LabelValues<Labels> { return {} as LabelValues<Labels> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.ts` around lines 44 - 48, Add a JSDoc comment to the protected method getLabelValuesForProcessedMessage (in PrometheusMessageCounter) explaining that the default implementation returns an empty LabelValues and that subclasses MUST override this method when the generic Labels type is not never to supply values for any registered labelNames; mention that when Labels is never the empty object is valid and that implementations should accept a ProcessedMessageMetadata<MessagePayload> parameter and return LabelValues<Labels> populated for the metric's labelNames.packages/metrics/lib/prometheus/metrics/message-time/PrometheusMessageTimeMetric.ts (1)
29-35: Label order differs fromPrometheusMessageCounter.
PrometheusMessageTimeMetricuses['messageType', 'version', 'queue', 'result', ...]whilePrometheusMessageCounteruses['queue', 'messageType', 'version', 'result', ...]. While Prometheus doesn't care about label order, inconsistency could confuse developers inspecting raw metrics. Consider aligning the order across metric types.♻️ Align label order with counter
labelNames: [ - 'messageType', - 'version', 'queue', + 'messageType', + 'version', 'result', ...(this.metricParams.labelNames ?? []), ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/metrics/lib/prometheus/metrics/message-time/PrometheusMessageTimeMetric.ts` around lines 29 - 35, PrometheusMessageTimeMetric's labelNames array order is inconsistent with PrometheusMessageCounter; update the labelNames in PrometheusMessageTimeMetric to match the counter's ordering by putting 'queue' first (i.e., use ['queue', 'messageType', 'version', 'result', ...(this.metricParams.labelNames ?? [])]) so both metrics share the same label order; locate the labelNames declaration in PrometheusMessageTimeMetric and adjust it to match PrometheusMessageCounter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.ts`:
- Around line 44-48: Add a JSDoc comment to the protected method
getLabelValuesForProcessedMessage (in PrometheusMessageCounter) explaining that
the default implementation returns an empty LabelValues and that subclasses MUST
override this method when the generic Labels type is not never to supply values
for any registered labelNames; mention that when Labels is never the empty
object is valid and that implementations should accept a
ProcessedMessageMetadata<MessagePayload> parameter and return
LabelValues<Labels> populated for the metric's labelNames.
In
`@packages/metrics/lib/prometheus/metrics/message-time/PrometheusMessageTimeMetric.ts`:
- Around line 29-35: PrometheusMessageTimeMetric's labelNames array order is
inconsistent with PrometheusMessageCounter; update the labelNames in
PrometheusMessageTimeMetric to match the counter's ordering by putting 'queue'
first (i.e., use ['queue', 'messageType', 'version', 'result',
...(this.metricParams.labelNames ?? [])]) so both metrics share the same label
order; locate the labelNames declaration in PrometheusMessageTimeMetric and
adjust it to match PrometheusMessageCounter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2fd7d78-82d5-418b-b071-2be065e3d13d
📒 Files selected for processing (10)
packages/metrics/README.mdpackages/metrics/lib/index.tspackages/metrics/lib/prometheus/PrometheusMessageMetric.tspackages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.spec.tspackages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.tspackages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageErrorCounter.spec.tspackages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageResultCounter.spec.tspackages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageResultCounter.tspackages/metrics/lib/prometheus/metrics/message-time/PrometheusMessageTimeMetric.tspackages/metrics/lib/prometheus/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/metrics/lib/prometheus/metrics/message-error/PrometheusMessageCounter.spec.ts
kjamrog
approved these changes
Mar 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add custom label support to Prometheus metric base classes
Both
PrometheusMessageTimeMetricandPrometheusMessageCounternow accept an optionalLabelstype parameter that allows subclasses to register additional Prometheus labels beyond the built-in defaults.The
labelNamesparam is required when Labels is specified, and omitted entirely when it's not, enforced at the type level via a conditional type, so there's no runtime overhead and no breaking change for existing implementations.Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests