Add OTel ack/dropped metrics and collapse inbox trace tags

[mtmk]

Three OpenTelemetry follow-ups on the 3.x metrics work: record the operation.duration histogram for JetStream acks, add a dropped-messages counter on the message-drop path, and collapse inbox subjects to a constant on the high-cardinality trace tags so backends don't index a unique value per request. The raw subject stays reachable through the Enrich callback.

Fixes #1158, #1162, #1157.

[synadia-claude-reviewer]

Claude finished @mtmk's task in 4m 46s —— View job

---

PR Review

• Read CLAUDE.md for repo guidelines
• Fetch PR diff
• Review Telemetry.cs changes
• Review NatsConnection.cs changes
• Review NatsJSMsg.cs changes
• Review test changes
• Post final review

---

Summary

The three changes (ack duration histogram, dropped-messages counter, inbox subject collapsing) are all correct and well-implemented. No bugs, no protocol issues. A few things worth the author's attention:

conn.Opts.InboxPrefix vs conn.InboxPrefix in LowCardinalitySubject (inline comment on Telemetry.cs:362)

conn.Opts.InboxPrefix (e.g. "_INBOX") and conn.InboxPrefix (e.g. "_INBOX.<nuid>") are different things — the former is the user-configured base prefix, the latter is this connection's unique per-connection inbox pattern. Using Opts.InboxPrefix in LowCardinalitySubject is correct (it must collapse inbox subjects from any connection, not only this one's own inbox), but it creates a visible asymmetry with the DestIsTemporary check on L262 which correctly uses conn.InboxPrefix. A short comment on the method explaining the deliberate choice would prevent a well-meaning cleanup from breaking it.

Inbox collapse is incomplete in the fallback branches of StartSendActivity/StartReceiveActivity

Both methods have an else branch (reached when ServerInfo is null or the connection is a custom INatsConnection wrapper) that still emits the raw subject. Since LowCardinalitySubject requires a concrete NatsConnection, this gap can't be closed without refactoring the signature to accept a plain string inboxPrefix. The impact is limited to pre-connect traces or custom wrapper types, so it's a known limitation rather than a blocker, but it's worth a // known gap comment on those branches so it doesn't look like an oversight.

All ack variants record with operation = "ack" (inline comment on NatsJSMsg.cs:266)

NakAsync, AckProgressAsync, and AckTerminateAsync all route through SendAckAsync and land in the same histogram bucket. If a consumer is NAK-ing heavily, that latency folds into the same signal as genuine acks with no way to tease them apart. This may be intentional (all ack-protocol messages are one operation type), but a comment stating so would help future readers.

DroppedMessages.Add is unguarded

The .Add() in OnMessageDropped has no try/catch, unlike RecordOperationDuration which swallows instrumentation exceptions deliberately. In practice Counter<long>.Add from .NET Meter shouldn't throw, and the existing PublishedMessages.Add/ConsumedMessages.Add calls elsewhere are also unguarded, so this is consistent — but worth noting if the defensive pattern is ever extended.

Overall the implementation is solid. The test coverage for all three features is thorough and the OTel semantic-convention decisions are well-commented.

[github-actions]

🚀 Deployed on https://6a3cf7a04bb726ce4b9485ed--teal-klepon-7e92d0.netlify.app

[mtmk]

@claude please re-review the latest commit. Addresses the two earlier review notes by documenting the intentional choices: inbox-prefix breadth in LowCardinalitySubject, and collapsing all ack-protocol kinds to one OpAck histogram.

[synadia-claude-reviewer]

Claude finished @mtmk's task in 40s —— View job

---

Re-review: c862217 — telemetry: document inbox-prefix and ack-op choices

• Read latest commit diff
• Check inbox-prefix documentation in Telemetry.cs
• Check ack-op documentation in NatsJSMsg.cs
• Post findings

---

Both previous review notes are addressed cleanly.

Telemetry.cs:361–363 — inbox-prefix breadth

The three-line addition to the existing comment block on LowCardinalitySubject is exactly right:

// Uses Opts.InboxPrefix (the bare prefix, e.g. "_INBOX") rather than conn.InboxPrefix
// (this connection's "_INBOX.<nuid>"): a reply-to address can belong to any connection,
// so the match must be broad. Do not "normalise" this to conn.InboxPrefix.

The explanation covers the what (Opts.InboxPrefix vs conn.InboxPrefix), the why (reply-to subjects cross connection boundaries), and the don't warning explicitly. No reviewer or future contributor should be tempted to change this.

NatsJSMsg.cs:236–239 — single-bucket ack histogram

// All ack-protocol messages (Ack, Nak, AckProgress, AckTerminate) route through here and
// record under a single OpAck operation, so their durations share one histogram. This is
// intentional: the operation tag tracks "sent an ack-protocol reply", not the ack kind.
// Do not split per kind without weighing the added metric cardinality.

Again, the comment is placed exactly where the decision is made (the top of SendAckAsync), names all four callers, states the intent, and flags the cardinality tradeoff as the reason not to split. This is the right amount of documentation for an intentional design choice.

No further issues. The PR is ready to merge.

[scottf]

LGTM