Record the RPC outcome on telemetry activities

[pepone]

Fixes #4807

The telemetry interceptor and middleware start an Activity per icerpc request, but the activity ended with its status
left Unset whatever the outcome: a response with a status code other than Ok, an exception thrown by the invocation
or the dispatch, and a cancellation all looked the same as a successful call in the exported traces, and no error.type
attribute was recorded. Trace backends render Unset as success, so failed RPCs did not show up as errors.

Both components now record the outcome before the activity stops, following the OpenTelemetry semantic conventions for
RPC spans and the "Recording errors" guidance. The rules differ by span kind, as they do for HTTP client and server
spans:

• The interceptor (client span) tags every response with rpc.status_code. A status code other than Ok sets the
  activity status to Error, with the response error message as the status description, and tags error.type with
  the status code name. An exception sets the activity status to Error with the exception message as the status
  description and tags error.type with the exception's fully qualified type name; no status code is recorded since
  there is no response.
• The middleware (server span) tags the activity with the rpc.status_code of the dispatch outcome: the status code of
  the returned response, or the status code the icerpc connection derives from an exception thrown by the dispatch
  (DispatchException and its ConvertToInternalError flag, InvalidDataException, NotSupportedException, truncated
  payloads, and InternalError for anything else). A dispatch canceled by the middleware's cancellation token records
  no status code, since the connection sends no response for it. A status code that reports a problem with the request
  (ApplicationError, NotFound, InvalidData, TruncatedPayload, Unauthorized) leaves the activity status unset,
  like a 4xx on an HTTP server span. Any other failure status code, or an exception, sets the activity status to Error
  and tags error.type with the status code name or the exception's fully qualified type name.
• A successful response leaves the status Unset and sets no error.type on both sides.

To let the middleware report the status code the caller receives without duplicating the connection's mapping, the
type switch in IceRpcProtocolConnection and DispatchException.ToOutgoingResponse are folded into
IceRpc.Internal.ExceptionExtensions (ToStatusCode and ToOutgoingResponse), and IceRpc makes its internals
visible to IceRpc.Telemetry. The response the connection sends is unchanged.

The RPC behavior is unchanged: the same response is returned and the same exception propagates.

New tests capture the activity from an ActivityListener when it stops and assert the status, the description, and
the tags for a failure response, for an exception, and for a successful response, on both the interceptor and the
middleware. The middleware tests cover every status code on both sides of the server-error line, and the exception
mapping including ConvertToInternalError and both kinds of cancellation.

What's Changed entry

Area: Telemetry

• The telemetry interceptor and middleware now record the outcome of each invocation and dispatch on the activity.
  Every activity that has a response carries an rpc.status_code tag. On the interceptor, a status code other than
  Ok or an exception sets the activity status to Error with an error.type tag. On the middleware, a status code
  that reports a server failure or an exception sets the activity status to Error, while a status code that reports
  a problem with the request, such as NotFound or Unauthorized, leaves it unset.

[Copilot]

🔵 Needs a closer look

Remaining documentation inconsistencies and non-unanimous readiness warrant final human review.

Pull request overview

Updates IceRPC telemetry to record RPC outcomes on activities for responses, exceptions, and cancellations.

Changes:

• Adds status and error tagging for failed RPC outcomes.
• Adds shared telemetry outcome helpers and documentation.
• Adds interceptor and middleware coverage for success, failure, and exception paths.

File summaries

File	Summary
tests/IceRpc.Telemetry.Tests/TelemetryMiddlewareTests.cs	Tests dispatch outcome telemetry.
tests/IceRpc.Telemetry.Tests/TelemetryInterceptorTests.cs	Tests invocation outcome telemetry.
src/IceRpc.Telemetry/TelemetryMiddleware.cs	Records dispatch outcomes before stopping activities.
src/IceRpc.Telemetry/TelemetryInterceptor.cs	Records invocation outcomes before stopping activities.
src/IceRpc.Telemetry/README.md	Documents telemetry outcome recording.
src/IceRpc.Telemetry/Internal/ActivityExtensions.cs	Centralizes activity status and tag handling.

Review details

Suppressed comments (2)

src/IceRpc.Telemetry/TelemetryInterceptor.cs:62

• The PR description says every activity carries rpc.response.status_code, but this exception path records only error.type; the new test also explicitly expects the status-code tag to be absent for exceptions. Since an exception may produce no response status, please either narrow that description to response outcomes or define an explicit status value instead of leaving the stated invariant contradictory.

            catch (Exception exception)
            {
                activity.RecordException(exception);

src/IceRpc.Telemetry/TelemetryMiddleware.cs:58

• The PR description says every activity carries rpc.response.status_code, but this exception path records only error.type; the new test also explicitly expects the status-code tag to be absent for exceptions. Since an exception may produce no response status, please either narrow that description to response outcomes or define an explicit status value instead of leaving the stated invariant contradictory.

            catch (Exception exception)
            {
                activity.RecordException(exception);

• Files reviewed: 6/6 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[externl]

rpc.response.status_code is already deprecated upstream: open-telemetry/semantic-conventions#3921 renamed it to rpc.status_code, and the registry on main lists the old name as replaced. It is unreleased as of v1.44.0, but I'd ship the new name rather than one the next release retires. The remarks, the README and the tests carry the old name too.

[pepone]

🤖 Claude: Done in d180690: the tag is now rpc.status_code in the code, the remarks and the tests. Checked against the semantic-conventions registry on main, which lists rpc.response.status_code as renamed to rpc.status_code. The README paragraph is gone, per Bernard's comment.

[externl]

HTTP draws this line by span kind: a client span records 4xx and 5xx as errors, which matches what the interceptor does, but a server span records only 5xx and unhandled exceptions. .NET follows that in HttpClient, in ASP.NET Core's built-in tracing and in the OpenTelemetry contrib instrumentation. Would it make sense for the middleware to do the same and leave the codes that report the caller's problem, such as NotFound, Unauthorized and InvalidData, unset?

[pepone]

🤖 Claude: Done in d180690. The middleware now leaves the activity status unset for ApplicationError, NotFound, InvalidData, TruncatedPayload and Unauthorized, while still recording rpc.status_code for them. NotImplemented, Unavailable, InternalError, DeadlineExceeded, NotSupported and unknown values set Error, mirroring the gRPC server list (UNIMPLEMENTED, UNAVAILABLE, INTERNAL, DEADLINE_EXCEEDED, UNKNOWN). TruncatedPayload sits on the request side because its documentation says it is usually caused by the invoker; ApplicationError because it is an application-defined outcome rather than a server failure. The middleware remarks list both sets. The interceptor is unchanged: every status code other than Ok is an error, as for HTTP client spans.

[bernardnormier]

Agreed with Joe on both points.

RecordStatusCode and RecordException are inconsistent: the first records the status code tag, the second
doesn't. Give RecordException a StatusCode? parameter, with null meaning no status code tag. The interceptor
passes null: an exception there means no response. The middleware passes the status code the caller will get, which
the protocol connection decides after the middleware returns (ConvertToInternalError, the mapping of
InvalidDataException and friends). Don't reproduce that mapping in the middleware: fold the two halves of the
conversion (the type switch in IceRpcProtocolConnection and ToOutgoingResponse) into one internal helper in
IceRpc, call it from both places, and add InternalsVisibleTo("IceRpc.Telemetry"). Internal because the mapping is
icerpc-specific: the ice connection maps only InvalidDataException. Please file an issue on whether the two
protocols should share one mapping, exposed publicly for any middleware that reports outcomes.

Please delete the new paragraph in the README: the README is a high-level description, and the tags are a detail
that belongs in the class remarks.

The new remarks sentence in both classes is hard to parse: four facts in one sentence with a nested parenthesis.
One sentence per fact, for example on the interceptor:

The activity records the outcome of the invocation. When the invocation returns a response, the
`rpc.status_code` tag holds its status code. When the status code is not `Ok` or the invocation throws an
exception, the activity status is `Error` and the `error.type` tag identifies the failure.

Two test nits, take them or leave them: Assert.That(outcome.Tags, Does.ContainKey("x").WithValue("y")) replaces
each ContainsKey/indexer pair, and Assert.CatchAsync replaces the try/catch that captures the thrown exception.

[pepone]

🤖 Claude: Applied in d180690:

• RecordException takes a StatusCode?. The interceptor passes null; the middleware passes the status code the
  icerpc connection derives from the exception. The type switch in IceRpcProtocolConnection and
  DispatchException.ToOutgoingResponse are folded into IceRpc.Internal.ExceptionExtensions (ToStatusCode and
  ToOutgoingResponse): the connection calls the fold, the middleware calls ToStatusCode, and IceRpc adds
  InternalsVisibleTo("IceRpc.Telemetry"). The ice connection keeps its own mapping and still calls
  ToOutgoingResponse on the DispatchException it builds. The protocol connection tests pass unchanged.
• A dispatch canceled by the middleware's cancellation token records no status code, mirroring the connection's
  OperationCanceledException filter: it sends no response in that case.
• Filed Share the dispatch exception to status code mapping between the ice and icerpc protocols #4954 on sharing the mapping between the two protocols and exposing it publicly.
• README paragraph removed. Remarks rewritten one sentence per fact, using your interceptor text.
• Tests use Does.ContainKey().WithValue() and Assert.CatchAsync.

[pepone]

🤖 Claude: Two policy questions on the middleware that d180690 leaves as reviewed, for @externl and
@bernardnormier:

1. Exceptions vs. equivalent responses. Every exception thrown by the dispatch sets the activity status to
   Error, per "5xx and unhandled exceptions". In IceRPC, exceptions also map to request-problem codes:
   throw new DispatchException(StatusCode.Unauthorized), or an InvalidDataException while decoding the payload,
   produces the same Unauthorized / InvalidData response as returning it, yet the thrown form is Error and the
   returned form is unset. If the derived status code should decide in both cases, the change is one line in the
   middleware's exception path, isError = statusCode is null || IsServerError(statusCode), which keeps every
   exception mapped to InternalError as an error.

2. Cancellation. A dispatch canceled by the middleware's cancellation token is recorded as Error with
   error.type = System.OperationCanceledException and no status code, matching how the PR treats cancellation on the
   interceptor (as HttpClient does). On the server side this cancellation means the connection is shutting down or
   the peer went away, not a server failure, and the HTTP conventions (Development status) say a caller-requested
   cancellation should not be an error. Should the middleware leave the status unset in that case, and should the
   interceptor do the same when the token passed to InvokeAsync is the one that fired?

[Copilot]

🟡 Changes recommended

Unknown status codes can create unbounded error.type cardinality and need a bounded fallback.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Balanced