Skip to content

fix(telemetry): preserve built-in meter discovery#452

Merged
yordis merged 1 commit into
masterfrom
yordis/fix-telemetry-instrumentation-contract
Jul 20, 2026
Merged

fix(telemetry): preserve built-in meter discovery#452
yordis merged 1 commit into
masterfrom
yordis/fix-telemetry-instrumentation-contract

Conversation

@yordis

@yordis yordis commented Jul 20, 2026

Copy link
Copy Markdown
Member
  • Built-in database telemetry must remain discoverable when operators configure additional meter sources.
  • Canonical instrumentation metadata prevents scope drift without changing emitted metrics.
  • Instrumentation version metadata must remain self-contained across published layouts.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@cursor

cursor Bot commented Jul 20, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Observability registration and configuration only; emitted metric names and instrumentation logic are unchanged, with new unit tests covering meter list composition.

Overview
Built-in Core and Projections meters are always subscribed for OpenTelemetry/Prometheus, regardless of what operators put in metricsconfig.json Meters. The config list is now only for extra System.Diagnostics.Metrics sources; the sample config uses an empty array and the metrics docs describe the new behavior.

A new TelemetryMeterInstrumentation helper centralizes canonical meter names (EventStore.Core, EventStore.Projections.Core), GetNames (built-ins first, deduped, validated extras), and scope version from the assembly informational version. TelemetryMeterFactory, MetricsBootstrapper, and ProjectionsSubsystem use those constants instead of string literals; ClusterVNodeStartup calls GetNames(metricsConfiguration.Meters) when registering meters.

Unit tests cover GetNames ordering/deduping/validation and shared scope versioning.

Reviewed by Cursor Bugbot for commit ce46a91. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d1b1d870-38d3-4a3b-ad3d-fa1973b72370

📥 Commits

Reviewing files that changed from the base of the PR and between 198b648 and ce46a91.

📒 Files selected for processing (9)
  • docs/diagnostics/metrics.md
  • src/EventStore.ClusterNode/metricsconfig.json
  • src/EventStore.Core.XUnit.Tests/OpenTelemetry/TelemetryMeterFactoryTests.cs
  • src/EventStore.Core.XUnit.Tests/OpenTelemetry/TelemetryMeterInstrumentationTests.cs
  • src/EventStore.Core/ClusterVNodeStartup.cs
  • src/EventStore.Core/Diagnostics/TelemetryMeterFactory.cs
  • src/EventStore.Core/Diagnostics/TelemetryMeterInstrumentation.cs
  • src/EventStore.Core/MetricsBootstrapper.cs
  • src/EventStore.Projections.Core/ProjectionsSubsystem.cs

Walkthrough

Centralizes built-in telemetry meter names and instrumentation scope version handling, updates metrics registration to include built-in meters automatically, and adds tests for naming, version parsing, ordering, deduplication, and validation.

Changes

Telemetry instrumentation

Layer / File(s) Summary
Instrumentation names and scope version
src/EventStore.Core/Diagnostics/TelemetryMeterInstrumentation.cs, src/EventStore.Core/Diagnostics/TelemetryMeterFactory.cs, src/EventStore.Core/MetricsBootstrapper.cs, src/EventStore.Projections.Core/ProjectionsSubsystem.cs
Shared constants and scope version parsing replace hard-coded meter names and reflection-based version lookup.
Built-in meter registration
src/EventStore.Core/ClusterVNodeStartup.cs, src/EventStore.ClusterNode/metricsconfig.json, docs/diagnostics/metrics.md
Metrics setup always registers Core and Projections meters while configuration supplies additional meter names.
Instrumentation behavior tests
src/EventStore.Core.XUnit.Tests/OpenTelemetry/*
Tests cover names, version parsing, ordering, deduplication, and invalid inputs.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant metricsconfig.json
  participant ConfigureMetrics
  participant TelemetryMeterInstrumentation
  participant MeterProviderBuilder
  metricsconfig.json->>ConfigureMetrics: Meters array
  ConfigureMetrics->>TelemetryMeterInstrumentation: GetNames(additional meter names)
  TelemetryMeterInstrumentation-->>ConfigureMetrics: Built-in and additional names
  ConfigureMetrics->>MeterProviderBuilder: AddMeter(names)
Loading

Possibly related PRs

Poem

A bunny found two meters bright,
Core and Projections, hopped in sight.
Extra names join the measured array,
Clean versions guide the way.
Tests keep every scope just right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: preserving built-in meter discovery.
Description check ✅ Passed The description matches the PR goals by describing built-in telemetry discovery, canonical metadata, and self-contained version metadata.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch yordis/fix-telemetry-instrumentation-contract

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Biome (2.5.3)
src/EventStore.ClusterNode/metricsconfig.json

File contains syntax errors that prevent linting: Line 2: Expected a property but instead found '// must be 0, 1, 5, 10 or a multiple of 15'.; Line 3: End of file expected; Line 3: End of file expected; Line 3: End of file expected; Line 3: End of file expected; Line 7: End of file expected; Line 7: End of file expected; Line 7: End of file expected; Line 9: End of file expected; Line 11: End of file expected; Line 11: End of file expected; Line 11: End of file expected; Line 11: End of file expected; Line 13: End of file expected; Line 13: End of file expected; Line 13: End of file expected; Line 17: End of file expected; Line 19: End of file expected; Line 19: End of file expected; Line 19: End of file expected; Line 19: End of file expected; Line 21: End of file expected; Line 21: End of file expected; Line 21: End of file expected; Line 21: End of file expected; Line 23: End of file expected; Line 23: End of file expected; Line 23: End of file expected; Line 23: End of file expecte

... [truncated 1326 characters] ...

expected; Line 114: End of file expected; Line 116: End of file expected; Line 116: End of file expected; Line 119: Expected a property but instead found '// this is the most expensive metric, creating a histogram for each'.; Line 116: End of file expected; Line 119: End of file expected; Line 121: End of file expected; Line 121: End of file expected; Line 121: End of file expected; Line 122: End of file expected; Line 124: End of file expected; Line 124: End of file expected; Line 124: End of file expected; Line 150: End of file expected; Line 154: End of file expected; Line 154: End of file expected; Line 154: End of file expected; Line 203: End of file expected; Line 205: End of file expected; Line 205: End of file expected; Line 205: End of file expected; Line 279: End of file expected


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@yordis
yordis merged commit 716895e into master Jul 20, 2026
23 checks passed
@yordis
yordis deleted the yordis/fix-telemetry-instrumentation-contract branch July 20, 2026 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant