Skip to content

Fix server.port type and trace tag source#1175

Merged
mtmk merged 2 commits into
release/3.0from
otel-server-port-int-tag
Jun 2, 2026
Merged

Fix server.port type and trace tag source#1175
mtmk merged 2 commits into
release/3.0from
otel-server-port-int-tag

Conversation

@mtmk

@mtmk mtmk commented Jun 1, 2026

Copy link
Copy Markdown
Member

Emit server.port and network.peer.port as integers per the OTel messaging semantic conventions instead of strings, and source server.address/server.port from the connect URI rather than the server bind address (which is often 0.0.0.0). The host, boxed port and client id are cached per ServerInfo change so the per-message trace path avoids re-boxing and re-allocating.

Fixes #1167
Fixes #1156

server.port and network.peer.port are integers per OTel semconv but the trace
activity tags emitted them via ToString(). server.address/server.port also read
from ServerInfo.Host/Port, which is the server bind address (often 0.0.0.0), not
the endpoint the client dialled.

Source the trace host and port from the connect URI to match the metric tags,
and cache the host, boxed port and client id once per ServerInfo change so the
per-message path no longer re-boxes the port or re-allocates the client id
string.
@mtmk mtmk added this to the Release 3.0 milestone Jun 1, 2026
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

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


PR Review

  • Gather context and read diff
  • Read relevant source files
  • Analyze for correctness, thread-safety, and performance
  • Post review findings

Overall: Correct and well-reasoned. The memory ordering is sound, the OTel spec fix is right, and the caching optimization is a genuine improvement. One pre-existing NRE risk is worth noting.

Memory ordering — correct

The WritableServerInfo setter writes the three cached fields with Volatile.Write (release semantics) and then publishes _writableServerInfo last with Interlocked.Exchange (full fence). Any reader that observes the new ServerInfo through the acquire-read in Interlocked.CompareExchange is guaranteed to also see the matching _boxedServerPort, _serverHost, and _clientId. The ordering is correct.

OTel spec conformance — correct

server.port and network.peer.port are now emitted as boxed int (from connectUri?.Port ?? value.Port, which resolves to int). This matches the OTel messaging semconv requirement. The test addition using Assert.IsType<int>(tag) correctly validates this.

Address source fix — correct

Using connectUri?.Host/connectUri?.Port as the primary source for server.address/server.port, falling back to ServerInfo.Host/ServerInfo.Port when no connect URI is available. This fixes the 0.0.0.0 emission issue (#1156).

Pre-existing NRE risk in Telemetry.cs (not introduced here)

In StartSendActivity (line 156) and StartReceiveActivity (line 268), conn.ServerInfo.ClientIp re-reads the live ServerInfo property after the pattern match guard:

// guard passes here — ServerInfo is non-null
if (connection is NatsConnection { ServerInfo: not null } conn)
{
    ...
    tags[10] = new KeyValuePair<string, object?>(Constants.NetworkLocalAddress, conn.ServerInfo.ClientIp);
    //                                                                           ^^^^^^^^^^^^ re-read, can race to null

During disconnect, _writableServerInfo is set to null last (after the cached fields), so there is a narrow window where the guard passes but conn.ServerInfo returns null on the re-read, yielding an NRE. The old code had the same problem with conn.ServerInfo.Port. Now that ClientIp is the only remaining live-read field, it would be straightforward to cache it alongside ClientId if desired. Not a blocker for this PR.

Null-tag window during disconnect — minor, actually improved

When WritableServerInfo is set to null, the cached fields are nulled before _writableServerInfo. A reader that just passes { ServerInfo: not null } can see null for BoxedServerPort/ServerHost/ClientId. This results in null tag values rather than the old NRE on conn.ServerInfo.Port. Net improvement.

Caching efficiency

One int box and one clientId.ToString() allocation per ServerInfo change; zero extra allocations on the per-message hot path. The approach is correct and proportionate.

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown

@mtmk
mtmk requested a review from scottf June 1, 2026 08:52

@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