-
Notifications
You must be signed in to change notification settings - Fork 376
[WCF] Fix exception when no endpoint action #4026
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| using System.ServiceModel; | ||
| using System.ServiceModel.Channels; | ||
| using System.ServiceModel.Description; | ||
| using System.ServiceModel.Dispatcher; | ||
| using Xunit; | ||
|
|
||
| namespace OpenTelemetry.Instrumentation.Wcf.Tests; | ||
|
|
||
| public class TelemetryEndpointBehaviorTests | ||
| { | ||
| [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(); | ||
| } | ||
| } | ||
|
Comment on lines
+14
to
+35
|
||
|
|
||
| private sealed class InjectNullActionOperationBehavior : IEndpointBehavior | ||
| { | ||
| public void AddBindingParameters(ServiceEndpoint endpoint, BindingParameterCollection bindingParameters) | ||
| { | ||
| // No-op | ||
| } | ||
|
|
||
| public void ApplyClientBehavior(ServiceEndpoint endpoint, ClientRuntime clientRuntime) | ||
| { | ||
| var nullActionOp = new ClientOperation(clientRuntime, "NullActionOperation", null); | ||
| clientRuntime.ClientOperations.Add(nullActionOp); | ||
| } | ||
|
|
||
| public void ApplyDispatchBehavior(ServiceEndpoint endpoint, EndpointDispatcher endpointDispatcher) | ||
| { | ||
| // No-op | ||
| } | ||
|
|
||
| public void Validate(ServiceEndpoint endpoint) | ||
| { | ||
| // No-op | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new null-check avoids the exception, but skipping
clientOperation.Action == null(anddispatchOperation.Action == null) means actionless operations are never added toactionMappings. The runtime instrumentation normalizes missing/empty SOAP action tostring.Empty(seeClientChannelInstrumentation.BeforeSendRequest/TelemetryDispatchMessageInspector.AfterReceiveRequest), so spans for these operations will end up withoutrpc.service/rpc.methodmetadata. Consider mapping null actions tostring.Empty(with collision handling) instead ofcontinue, so actionless endpoints still get meaningful tags.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martincostello, what do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.