Skip to content

[Instrumentation.ServiceFabricRemoting] Support custom exception convertors - #5166

Merged
martincostello merged 6 commits into
open-telemetry:mainfrom
sablancoleis:sfr/exception-serialization
Sep 9, 2026
Merged

martincostello merged 6 commits into
open-telemetry:mainfrom
sablancoleis:sfr/exception-serialization

Conversation

@sablancoleis

@sablancoleis sablancoleis commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Changes

Service Fabric lets applications register IExceptionConvertor implementations
so that custom exception types survive a remoting call. The provider attributes
in this package build the listener and the client factory internally and call
the Service Fabric overloads that take no convertors, so applications using them
have no way to opt in.

This became a problem in Service Fabric SDK 8 (runtime 11), which stopped
enabling the BinaryFormatter fallback for remoting exception serialization by
default; SDK 9 (runtime 12) removes it altogether. See the
Remoting V1 deprecation strategy.
Up to SDK 7.1 the defaults were BinaryFormatter on the listener and Fallback
on the client, which round-tripped any [Serializable] exception with no
configuration. From SDK 8 an exception type is only preserved if a convertor is
registered for it; anything else reaches the client as a ServiceException
wrapped in an AggregateException, so catch (MyCustomException) stops
matching.

  • Unseal TraceContextEnrichedServiceRemotingProviderAttribute and
    TraceContextEnrichedActorRemotingProviderAttribute and add
    GetServiceExceptionConvertors() / GetClientExceptionConvertors() hooks,
    passing the results to the Service Fabric listener and client factory
    overloads that accept exception convertors. This extends the existing Service
    Fabric provider-attribute model rather than introducing a new abstraction, and
    keeps real convertor instances so they can take dependencies.
  • Add a RemotingExceptionDepth property to control how many levels of inner
    exceptions are serialized.
  • Document the manual composition path in the README. Both adapters are already
    public, so applications needing full control over listener or client factory
    construction can wrap their own objects directly. This was previously
    undocumented.

Behaviour is unchanged by default: the hooks return null, and
RemotingExceptionDepth only overrides the Service Fabric default when set.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

…ertors

Service Fabric SDK 8 (runtime 11) stopped enabling the BinaryFormatter fallback
for remoting exception serialization by default, and SDK 9 (runtime 12) removes
it altogether. See the Remoting V1 deprecation strategy:
https://github.com/microsoft/service-fabric/blob/master/release_notes/Deprecated/RemotingV1.md

Up to SDK 7.1 the defaults were BinaryFormatter on the listener and Fallback on
the client, which round-tripped any [Serializable] exception with no
configuration. From SDK 8 both default to data contract serialization, so an
exception type is only preserved if an IExceptionConvertor is registered for it;
anything else reaches the client as a ServiceException wrapped in an
AggregateException, and "catch (MyCustomException)" stops matching.

The provider attributes built the listener and the client factory internally and
called the Service Fabric overloads that take no convertors, so applications
using them had no way to opt in.

- Unseal both provider attributes and add GetServiceExceptionConvertors() and
  GetClientExceptionConvertors() hooks, passing the results to the Service
  Fabric listener and client factory overloads that accept exception convertors.
  This extends the existing Service Fabric provider-attribute model rather than
  introducing a new abstraction, and keeps real convertor instances so they can
  take dependencies.
- Add a RemotingExceptionDepth property to control how many levels of inner
  exceptions are serialized.
- Document the manual composition path in the README. Both adapters are already
  public, so applications needing full control over listener or client factory
  construction can wrap their own objects directly. This was previously
  undocumented.

Behaviour is unchanged by default: the hooks return null, and
RemotingExceptionDepth only overrides the Service Fabric default when set.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b9928e0c-3bbc-46f5-9ad5-55b0a69b4a44
@sablancoleis
sablancoleis requested a review from a team as a code owner September 2, 2026 21:55
@github-actions github-actions Bot added the comp:instrumentation.servicefabricremoting Things related to OpenTelemetry.Instrumentation.ServiceFabricRemoting label Sep 2, 2026
@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

Pull request dashboard status

Merged · refreshed 2026-09-10 17:10 UTC

Status above doesn't look right?
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

@codecov

codecov Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 23.07692% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.98%. Comparing base (49bb311) to head (103605f).
⚠️ Report is 13 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ceContextEnrichedActorRemotingProviderAttribute.cs 15.38% 11 Missing ⚠️
...ContextEnrichedServiceRemotingProviderAttribute.cs 30.76% 9 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5166      +/-   ##
==========================================
- Coverage   79.03%   78.98%   -0.05%     
==========================================
  Files         499      499              
  Lines       20941    20890      -51     
==========================================
- Hits        16551    16501      -50     
+ Misses       4390     4389       -1     
Flag Coverage Δ
unittests-Instrumentation.ServiceFabricRemoting 38.82% <23.07%> (-1.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ContextEnrichedServiceRemotingProviderAttribute.cs 37.87% <30.76%> (-0.31%) ⬇️
...ceContextEnrichedActorRemotingProviderAttribute.cs 11.32% <15.38%> (+1.79%) ⬆️

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

sablancoleis and others added 3 commits September 2, 2026 15:19
…tests

The actor provider attribute had no coverage for the new exception convertor
hooks. Mirror the service provider tests so that both attributes exercise the
default (no convertors registered) and derived (custom convertors registered)
paths, and the RemotingExceptionDepth property.

The listener creation and client factory construction in both attributes remain
uncovered because they instantiate Service Fabric transport objects that require
the Service Fabric runtime, which is not available on the CI agents.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b9928e0c-3bbc-46f5-9ad5-55b0a69b4a44
An editing mistake joined the opening brace with the first constructor, which
StyleCop flagged as SA1500 and SA1025 and broke the build.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b9928e0c-3bbc-46f5-9ad5-55b0a69b4a44
Comment thread src/OpenTelemetry.Instrumentation.ServiceFabricRemoting/CHANGELOG.md Outdated
Comment thread src/OpenTelemetry.Instrumentation.ServiceFabricRemoting/README.md Outdated
sablancoleis and others added 2 commits September 7, 2026 09:54
- Remove the documentation entry from the CHANGELOG, per review feedback that
  documentation-only changes are not usually listed there.
- Drop the redundant empty argument list from the assembly attribute example.
- Note that the derived attribute still accepts the Service Fabric settings
  inherited from the base attribute.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b9928e0c-3bbc-46f5-9ad5-55b0a69b4a44
@martincostello
martincostello added this pull request to the merge queue Sep 9, 2026
Merged via the queue into open-telemetry:main with commit ebd0513 Sep 9, 2026
76 checks passed
This was referenced Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:instrumentation.servicefabricremoting Things related to OpenTelemetry.Instrumentation.ServiceFabricRemoting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants