feat(telemetry): align metrics with OpenTelemetry conventions#453
Conversation
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryHigh Risk Overview A local Weaver registry ( Runtime wiring is updated to pass
Reviewed by Cursor Bugbot for commit 0e18038. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (74)
WalkthroughAdds an OpenTelemetry metric registry and generated C# ChangesMetric catalog and generation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SemconvTask
participant LocalRegistry
participant Weaver
participant MetricNames
SemconvTask->>LocalRegistry: validate pinned manifest
SemconvTask->>Weaver: generate metric conventions
Weaver->>MetricNames: emit constants and All
SemconvTask->>MetricNames: validate generated artifacts
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (2)
src/EventStore.Core/ClusterVNodeStartup.cs (1)
320-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSuffix-based histogram bucket selection still remains (unchanged).
This block still classifies histograms by
i.Name.EndsWith("-latency-seconds")/"-seconds"string suffixes — the same "hidden suffix rule" pattern this PR's objective calls out for removal. It's unchanged here, but the migration to explicitMetricNamesconstants (line 308) is incomplete relative to that stated goal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/EventStore.Core/ClusterVNodeStartup.cs` around lines 320 - 339, Replace the suffix-based checks in the histogram bucket selection logic with explicit comparisons against the appropriate MetricNames constants, including the latency-seconds and seconds metrics. Preserve the existing bucket configurations and ensure both branches continue selecting the same histogram boundaries.otel/semconv/templates/registry/csharp/metric-names.cs.j2 (1)
10-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated name-derivation filter chain to avoid drift between const declarations and the
Alllist.
metric.metric_name | pascal_case | regex_replace("^Eventstore", "")is duplicated verbatim at line 12 and line 20. Since line 20 must produce the exact identifier declared at line 12 to compile, any future change to this transform in only one place will break the build or silently desync theAlllist from the declared consts. Consider binding the result to a Jinja variable once per metric and reusing it in both loops.Separately, please confirm
pascal_caseandregex_replaceare filters actually provided by the pinned Weaver/MiniJinja version — Weaver's documented custom filters (prometheus_*, kebab_case_const, etc.) don't clearly list these two in what's publicly documented.♻️ Proposed DRY refactor
public static class MetricNames { {% for group in ctx %} -{% for metric in group.metrics | sort(attribute="metric_name") %} - public const string {{ metric.metric_name | pascal_case | regex_replace("^Eventstore", "") }} = "{{ metric.metric_name }}"; +{% for metric in group.metrics | sort(attribute="metric_name") %} +{% set const_name = metric.metric_name | pascal_case | regex_replace("^Eventstore", "") %} + public const string {{ const_name }} = "{{ metric.metric_name }}"; {% endfor %} {% endfor %} public static IReadOnlyList<string> All { get; } = Array.AsReadOnly(new[] { {% for group in ctx %} -{% for metric in group.metrics | sort(attribute="metric_name") %} - {{ metric.metric_name | pascal_case | regex_replace("^Eventstore", "") }}, +{% for metric in group.metrics | sort(attribute="metric_name") %} +{% set const_name = metric.metric_name | pascal_case | regex_replace("^Eventstore", "") %} + {{ const_name }}, {% endfor %} {% endfor %} });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otel/semconv/templates/registry/csharp/metric-names.cs.j2` around lines 10 - 22, Centralize the metric identifier derivation used by the const declarations and `All` list so both outputs reuse the same value and cannot drift; use a Jinja macro or shared binding appropriate to the template scope around the `metric.metric_name` loops. Verify that the pinned Weaver/MiniJinja version provides `pascal_case` and `regex_replace`; if either is unavailable, replace it with the supported equivalent while preserving the existing identifier transformation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/EventStore.Core.XUnit.Tests/Metrics/ProcessMetricsTests.cs`:
- Around line 67-72: Update the configuration passed to CreateDiskOpsMetric in
ProcessMetricsTests so it uses MetricsConfiguration.ProcessTracker.DiskReadOps
and DiskWrittenOps, while leaving the existing disk-bytes metric configuration
unchanged.
In `@src/EventStore.Core/MetricsBootstrapper.cs`:
- Around line 351-355: Update the expected literal for
MetricsConfiguration.ProcessTracker.GcPauseDuration in ProcessMetricsTests to
use the catalog name eventstore-gc-pause-duration-max-seconds, matching
MetricNames.GcPauseDurationMaxSeconds and production output.
---
Nitpick comments:
In `@otel/semconv/templates/registry/csharp/metric-names.cs.j2`:
- Around line 10-22: Centralize the metric identifier derivation used by the
const declarations and `All` list so both outputs reuse the same value and
cannot drift; use a Jinja macro or shared binding appropriate to the template
scope around the `metric.metric_name` loops. Verify that the pinned
Weaver/MiniJinja version provides `pascal_case` and `regex_replace`; if either
is unavailable, replace it with the supported equivalent while preserving the
existing identifier transformation.
In `@src/EventStore.Core/ClusterVNodeStartup.cs`:
- Around line 320-339: Replace the suffix-based checks in the histogram bucket
selection logic with explicit comparisons against the appropriate MetricNames
constants, including the latency-seconds and seconds metrics. Preserve the
existing bucket configurations and ensure both branches continue selecting the
same histogram boundaries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbc1e4c7-bbe9-4f20-8d8f-ea83ab0a915e
⛔ Files ignored due to path filters (1)
src/TrogonEventStore.SemanticConventions/Generated/MetricNames.g.csis excluded by!**/generated/**
📒 Files selected for processing (29)
.config/mise/tasks/semconv/check.config/mise/tasks/semconv/generateotel/semconv/registry/manifest.yamlotel/semconv/registry/trogon/eventstore/metrics.yamlotel/semconv/templates/registry/csharp/metric-names.cs.j2otel/semconv/templates/registry/csharp/weaver.yamlsrc/EventStore.Core.XUnit.Tests/Index/IndexTrackerTests.cssrc/EventStore.Core.XUnit.Tests/Metrics/AverageMetricTests.cssrc/EventStore.Core.XUnit.Tests/Metrics/CacheResourcesTrackerTests.cssrc/EventStore.Core.XUnit.Tests/Metrics/DurationMaxTrackerTests.cssrc/EventStore.Core.XUnit.Tests/Metrics/DurationTrackerTests.cssrc/EventStore.Core.XUnit.Tests/Metrics/ElectionsCounterTrackerTests.cssrc/EventStore.Core.XUnit.Tests/Metrics/ProcessMetricsTests.cssrc/EventStore.Core.XUnit.Tests/Metrics/QueueBusyTrackerTests.cssrc/EventStore.Core.XUnit.Tests/Metrics/QueueProcessingTrackerTests.cssrc/EventStore.Core.XUnit.Tests/Metrics/SystemMetricsTests.cssrc/EventStore.Core.XUnit.Tests/OpenTelemetry/MetricNamesTests.cssrc/EventStore.Core.XUnit.Tests/TransactionLog/Chunks/TFChunkTrackerTests.cssrc/EventStore.Core/ClusterVNodeStartup.cssrc/EventStore.Core/Metrics/AverageMetric.cssrc/EventStore.Core/Metrics/CacheResourcesMetrics.cssrc/EventStore.Core/Metrics/CounterMetric.cssrc/EventStore.Core/Metrics/DurationMaxMetric.cssrc/EventStore.Core/Metrics/DurationMetric.cssrc/EventStore.Core/Metrics/ProcessMetrics.cssrc/EventStore.Core/Metrics/SystemMetrics.cssrc/EventStore.Core/MetricsBootstrapper.cssrc/EventStore.Projections.Core/ProjectionsSubsystem.cssrc/TrogonEventStore.SemanticConventions/README.md
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e0f2dac. Configure here.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
1 participant
Uh oh!
There was an error while loading. Please reload this page.