[WCF] Fix exception when no endpoint action#4026
[WCF] Fix exception when no endpoint action#4026martincostello wants to merge 2 commits intoopen-telemetry:mainfrom
Conversation
Fix `ArgumentNullException` when an endpoint's operation action is null. Fixes open-telemetry#2584.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4026 +/- ##
==========================================
+ Coverage 71.94% 72.09% +0.14%
==========================================
Files 458 448 -10
Lines 17872 17823 -49
==========================================
- Hits 12858 12849 -9
+ Misses 5014 4974 -40
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Add PR number.
There was a problem hiding this comment.
Pull request overview
Fixes a WCF instrumentation crash when an operation’s SOAP Action is missing by preventing TelemetryEndpointBehavior from inserting null keys into its action-to-metadata map (addressing scenarios like WCF Web HTTP help pages).
Changes:
- Skip operations with
nullAction when building action mappings for both client and service paths. - Add a unit test ensuring
ChannelFactory.Open()does not throw when a client operation hasnullAction. - Document the fix in the WCF instrumentation changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/OpenTelemetry.Instrumentation.Wcf.Tests/TelemetryEndpointBehaviorTests.cs | Adds coverage for the client-side null Action scenario. |
| src/OpenTelemetry.Instrumentation.Wcf/TelemetryEndpointBehavior.cs | Avoids ArgumentNullException by handling null Action values when building action mappings. |
| src/OpenTelemetry.Instrumentation.Wcf/CHANGELOG.md | Records the bug fix in Unreleased notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (clientOperation.Action == null) | ||
| { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
The new null-check avoids the exception, but skipping clientOperation.Action == null (and dispatchOperation.Action == null) means actionless operations are never added to actionMappings. The runtime instrumentation normalizes missing/empty SOAP action to string.Empty (see ClientChannelInstrumentation.BeforeSendRequest / TelemetryDispatchMessageInspector.AfterReceiveRequest), so spans for these operations will end up without rpc.service/rpc.method metadata. Consider mapping null actions to string.Empty (with collision handling) instead of continue, so actionless endpoints still get meaningful tags.
There was a problem hiding this comment.
I wasn't sure if having an empty action was valid or not.
I can change it if it makes sense, but I figured it at least not throwing an exception and ignoring it was better than the current behaviour.
| [Fact] | ||
| public void ApplyClientBehaviorToClientRuntime_WithNullActionOperation_DoesNotThrow() | ||
| { | ||
| // Arrange | ||
| var contract = ContractDescription.GetContract(typeof(IServiceContract)); | ||
| var endpoint = new ServiceEndpoint(contract, new BasicHttpBinding(), new EndpointAddress("http://localhost/dummy")); | ||
|
|
||
| endpoint.EndpointBehaviors.Add(new InjectNullActionOperationBehavior()); | ||
| endpoint.EndpointBehaviors.Add(new TelemetryEndpointBehavior()); | ||
|
|
||
| var factory = new ChannelFactory<IServiceContract>(endpoint); | ||
|
|
||
| try | ||
| { | ||
| var exception = Record.Exception(factory.Open); | ||
| Assert.Null(exception); | ||
| } | ||
| finally | ||
| { | ||
| factory.Abort(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This test validates the client-side fix, but the reported crash in #2584 occurs in the server-side path (ApplyDispatchBehaviorToEndpoint) when a DispatchOperation.Action is null. Add a .NET Framework-only unit test covering ApplyDispatchBehaviorToEndpoint with a DispatchOperation whose Action is null to ensure the original failure mode stays covered.
Fixes #2584
Changes
Fix
ArgumentNullExceptionwhen an endpoint's operation action is null.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)