Skip to content

Exclude NATS status frames from consumed metrics#1195

Merged
mtmk merged 1 commit into
release/3.0from
otel-503-consumed-semconv
Jun 25, 2026
Merged

Exclude NATS status frames from consumed metrics#1195
mtmk merged 1 commit into
release/3.0from
otel-503-consumed-semconv

Conversation

@mtmk

@mtmk mtmk commented Jun 23, 2026

Copy link
Copy Markdown
Member

consumed.messages and received.bytes now count only messages delivered to the application, per the OpenTelemetry messaging convention. NATS status and control frames (no-responder 503s, JetStream heartbeats, flow-control, and protocol notifications) are consumed internally and were previously inflating both counters on the receive path.

Fixes #1168.

consumed.messages and received.bytes count only messages delivered to the
application, per the OTel messaging convention. NATS status/control frames
(no-responder 503s, JetStream heartbeats, flow-control, protocol notifications)
are consumed internally and never reach the application, but the base receive
path counted them. Skip them via the inline status code on the version line,
which covers every subscription type through one chokepoint.
@mtmk mtmk added this to the Release 3.0 milestone Jun 23, 2026
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 3m 20s —— View job


PR Review

  • Gather context
  • Read changed files
  • Analyze for correctness, security, and performance
  • Post inline comments
  • Write review summary

Summary

The implementation is correct and well-structured. A few observations:

IsStatusMsg correctness — The byte check at offset 8–11 (' ' + 3 ASCII digits) cleanly distinguishes status frames from regular user headers, which always have \r\n at offset 8. This supersedes the narrower IsHeader503 check in the telemetry path while IsHeader503 correctly stays in place for the ThrowIfNoResponders early-return.

ToSpan() on multi-segment sequences — The extension at BufferExtensions.cs:28 correctly handles both paths: single-segment returns GetFirstSpan() (zero alloc), multi-segment falls back to ToArray() (4-byte heap alloc). The new IsStatusMsg uses the same pattern as the pre-existing IsHeader503 call. Given that a 12-byte header slice almost always lives in a single segment, the fallback path is hypothetical.

Ordering: IsStatusMsg called after ReceiveInternalAsync — This is intentional and correct. Status frames are still processed by the subclass (e.g. heartbeats update internal state), they just aren't counted as "delivered to the application." The 503 case with ThrowIfNoResponders: true already exits before the telemetry block via the early-return at line 359, so there is no double-exclusion.

Tests — The unit tests in StatusMsgTest.cs cover all the named frame types and both negative cases. The integration test in OpenTelemetryTest.cs exercises the real JetStream fetch-with-expiry path and verifies the delta is exactly 3 (not 4 with the end-of-batch status frame).

One minor note: IsStatusMsg does not re-verify bytes 0–7 are NATS/1.0. This is safe because the NATS server guarantees the prefix, and IsHeader503 uses the same approach. Worth keeping in mind if any code path could ever inject a synthetic header buffer that doesn't follow protocol framing — but nothing in the current codebase does.

No correctness bugs, security issues, or performance concerns found.

@github-actions

Copy link
Copy Markdown

@mtmk
mtmk requested a review from scottf June 24, 2026 11:14
@mtmk mtmk added the ready label Jun 24, 2026

@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants