Skip to content

Add OTel metrics support#1154

Merged
mtmk merged 23 commits into
release/3.0from
otel-metrics-meter
May 30, 2026
Merged

Add OTel metrics support#1154
mtmk merged 23 commits into
release/3.0from
otel-metrics-meter

Conversation

@mtmk

@mtmk mtmk commented May 22, 2026

Copy link
Copy Markdown
Member

Adds OpenTelemetry metrics on the existing NATS.Net meter alongside
the activity source. Opt-in via AddNatsClientInstrumentation() on a
MeterProviderBuilder; no overhead without a listener.

Targets release/3.0. Refs #1153. Remaining: JetStream ack duration (#1158).

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 1m 51s —— View job


PR Review: Add OTel Meter and metric instruments

  • Gather context and read changed files
  • Review instrument names, units, and types
  • Check consistency with existing telemetry patterns
  • Post findings

The change is clean and minimal. Instrument names and types all match the OTel messaging metrics spec. Two items worth tracking:

HasListeners() scope (line 33 — informational for follow-ups)
The existing HasListeners() guard only checks NatsActivities.HasListeners() (tracing). If call sites in follow-up PRs use it to gate metric recording, metrics will be silently dropped even when an OTel SDK meter listener is active. The .NET metrics API is already a near-no-op with no listener, so metric call sites don't need a guard at all.

Reconnects unit (line 29 — minor)
"{event}" is valid UCUM annotation syntax but is generic. "{reconnect}" would be more self-describing and consistent with the pattern used by {message} and {subscription} in the other instruments.

No correctness or security issues found. Instrument types (Counter<long>, Histogram<double>, UpDownCounter<long>) are appropriate for their semantics, and sharing the "NATS.Net" instrumentation scope name between ActivitySource and Meter is the idiomatic .NET approach.

Comment thread src/NATS.Client.Core/Internal/Telemetry.cs
Comment thread src/NATS.Client.Core/Internal/Telemetry.cs Outdated
@github-actions

github-actions Bot commented May 22, 2026

Copy link
Copy Markdown

@mtmk mtmk changed the title Add OTel Meter and metric instruments Add OTel metrics support May 22, 2026
@mtmk
mtmk marked this pull request as ready for review May 26, 2026 09:54
mtmk added 2 commits May 26, 2026 11:04
The increment at PublishAsync entry fires before ConnectAsync completes
on a cold publish, so the first measurement is emitted with an empty
tag prefix (server.address/port unset). Move it next to sent.bytes
which is incremented post-serialization with the full tag set.
The previous implementation attached the handler after ConnectAsync, so
the async event-channel delivery of the initial ConnectionOpened could
satisfy the awaitable before the reconnect ran. Attach before connect
and count to two.
@mtmk mtmk added this to the Release 3.0 milestone May 26, 2026
@mtmk

mtmk commented May 26, 2026

Copy link
Copy Markdown
Member Author

@claude review

@mtmk

mtmk commented May 27, 2026

Copy link
Copy Markdown
Member Author

@claude review changes in this PR

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 5m 27s —— View job


PR Review: Add OTel metrics support

  • Gather context
  • Read changed files
  • Review instrument definitions and metric recording sites
  • Check test coverage
  • Post findings

The change is well-structured. Previous review concerns have been addressed ({reconnect} unit is used; the HasListeners() guard has a "Don't use this for metrics" comment; all metric recording sites use instrument .Enabled guards correctly). Below are the findings from this review pass.

What's confirmed correct

  • Instrument types and names match OTel messaging semconv: messaging.client.published.messages (Counter), messaging.client.consumed.messages (Counter), messaging.client.operation.duration (Histogram/s), nats.client.active_subscriptions (UpDownCounter), nats.client.reconnects (Counter), nats.client.sent.bytes/nats.client.received.bytes (Counters/By).
  • MetricTagsPrefix is thread-safe: set via Volatile.Write in the ServerInfo setter; read via Volatile.Read from MetricTagsPrefix. Connection tags are derived from the dialled URI, not ServerInfo.Host (which can be 0.0.0.0). ✓
  • SentBytes vs ReceivedBytes semantics are symmetric: both count serialized body + headers (payloadBuffer + headersBuffer). The docs table correctly says "(body + headers)". Wire-level NATS framing is excluded, which is appropriate for application-level byte accounting.
  • Active subscription counter (_telemetryActive + Interlocked.Exchange) correctly prevents double-increment/decrement regardless of which code path (explicit dispose vs. auto-end-subscription) reaches first. ✓
  • ReadyAsync() is called inside AddSubAsync, after the connection is established and MetricTagsPrefix is populated, so active-subscription measurements always carry full server tags. ✓
  • Reconnects counts only successful reconnects, not the initial connection and not failed attempts. The test correctly asserts a count of 1 after ConnectAsync + ReconnectAsync. ✓
  • NatsInstrumentationExtensions uses method overloading on AddNatsClientInstrumentation for both TracerProviderBuilder and MeterProviderBuilder, which is the idiomatic .NET OTel pattern. ✓

Findings

1. OperationDuration histogram — no advisory bucket boundaries (minor)

src/NATS.Client.Core/Internal/Telemetry.cs:22

public static readonly Histogram<double> OperationDuration =
    NatsMeter.CreateHistogram<double>("messaging.client.operation.duration", unit: "s");

The OTel semantic conventions for messaging.client.operation.duration explicitly recommend the SDK use these bucket boundaries: [0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10]. Without them, the OTel SDK falls back to its own defaults (typically the legacy HTTP buckets), which can make out-of-the-box dashboards look wrong for NATS operations.

On .NET 8+, this is one line:

public static readonly Histogram<double> OperationDuration =
    NatsMeter.CreateHistogram<double>(
        "messaging.client.operation.duration",
        unit: "s",
        advice: new InstrumentAdvice<double>
        {
            HistogramBucketBoundaries = [0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10],
        });

InstrumentAdvice<T> is in System.Diagnostics.Metrics (available netstandard2.0+ via the System.Diagnostics.DiagnosticSource package, which is already pulled in). Fix this →

2. server.port type inconsistency between trace and metric paths (informational, pre-existing)

src/NATS.Client.Core/Internal/Telemetry.cs:127

In trace tag arrays, ServerPort is stored as string (conn.ServerInfo.Port.ToString()). In the metric tag prefix (NatsConnection.cs), it is stored as int. The OTel spec defines server.port as integer. This is a pre-existing trace-side inconsistency; the metric path is spec-correct.

3. Reconnects — failed reconnect attempts not counted (documentation gap, informational)

The metric counts only successful reconnects (those that reach ConnectionState = Open). Failed reconnect attempts are silently excluded. The docs table says "Successful reconnects since process start" which is correct — just confirming the docs match the implementation. No fix needed, but worth being aware of if users ask why their reconnect count seems low.

4. Test gap: active subscriptions with connection close (minor)

tests/NATS.Net.OpenTelemetry.Tests/OpenTelemetryTest.cs

Active_subscriptions_updown_counter tests subscribe/dispose, but does not cover the case where the connection closes while subscriptions are active (i.e., whether the DecrementActiveSubscription path in NatsSubBase.CompleteAsync is exercised). The counter could appear stuck in those scenarios. Worth adding as a follow-up test.

5. PublishedMessages is recorded before the PUB frame is flushed (informational)

src/NATS.Client.Core/Commands/CommandWriter.cs:353

The counter is incremented after serialization and size-check pass, but before the data is actually flushed to the socket. A subsequent network failure would cause the count to be recorded while the message may not have been received by the broker. This is the typical client-side semantic (count-on-enqueue vs count-on-ack), and is consistent with how most client instrumentation libraries work. The docs table description ("Messages published by the client") is accurate. Just flagging for awareness.


Overall: The implementation is clean and production-ready as-is. Item 1 (advisory histogram buckets) is the one actionable improvement; items 2–5 are informational notes for awareness.

mtmk added 4 commits May 28, 2026 09:40
OTel messaging semconv recommends specific bucket boundaries for
messaging.client.operation.duration. Without them the SDK falls back to
HTTP-style defaults that have no resolution for sub-millisecond NATS
ops and skew published semconv dashboards.

InstrumentAdvice<T> requires System.Diagnostics.DiagnosticSource 9.0+,
so bump the package floor on netstandard2.0/2.1/net8.0. net10.0 keeps
using the in-box assembly. 9.0.1 is the minimum because 9.0.0 trims
the ActivitySource constructor under NativeAOT.
Behavior changes:
* Balance ActiveSubscriptions across listener-attach and ctor
  cancellation-token races so the gauge cannot drift negative or
  leak +1 per sub.
* Record operation.duration with error.type on synchronous publish
  throws (e.g. NatsPayloadTooLargeException, serializer faults) when
  no ActivityListener is attached.
* Count consume metrics only after the message is handed off to the
  subscription channel, matching OTel "delivered to the application"
  semantics. Late replies that arrive after a Direct mode timeout no
  longer inflate the counters. 503 NoResponders sentinels are
  excluded on the Direct path for parity with SharedInbox.
* Move Reconnects.Add out from under _gate so a slow MeterListener
  callback cannot stall the reconnect transition.
* Swallow exceptions inside RecordOperationDuration so a buggy
  listener cannot replace an in-flight messaging exception in the
  catch/finally wrappers around publish, request, and subscribe.

Refactor:
* Build the metric TagList once per hot-path site and reuse it
  across the published/sent and consumed/received counter pairs.
* Publish _metricTagsPrefix before _writableServerInfo so any reader
  observing the new ServerInfo is guaranteed to see the matching
  prefix.

Docs and comments:
* Clarify active_subscriptions semantics (includes SharedInbox
  muxer registrations created per RequestAsync), published.messages
  "attempted" semantics, and the rationale for the concrete-type
  check in BuildMetricTags.
@mtmk
mtmk requested a review from scottf May 30, 2026 06:03

@scottf scottf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

2 participants