WCF: action is assigned with null if operation name is not defined(#169)

[tamilaban]

public string DisplayName { get { return _displayName ?? OperationName; } set { _displayName = (value ?? throw new ArgumentNullException("value")); }

        Display name throws Argument null exception if action is null or operation contract name is defined in the WCF operationContract . 

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Tamilanban Chinnaiyan (d6575d1)

[CodeBlanch]

@tamilaban Thanks for the contribution! Would you mind adding a test that covers this case?

[codecov]

Codecov Report

Merging #170 (d7fff85) into main (e039516) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
+ Coverage   78.60%   78.62%   +0.01%     
==========================================
  Files          89       89              
  Lines        2323     2325       +2     
==========================================
+ Hits         1826     1828       +2     
  Misses        497      497              

Impacted Files	Coverage Δ
...rumentation.Wcf/TelemetryClientMessageInspector.cs	83.11% <100.00%> (+0.45%)	⬆️

[tamilaban]

@tamilaban Thanks for the contribution! Would you mind adding a test that covers this case?

HI @CodeBlanch , I could not make the action as null, I have assigned the action name to an empty string and updated the unit test in adherence . Can you please have a look ?

[tamilaban]

@tamilaban Thanks for the contribution! Would you mind adding a test that covers this case?

HI @CodeBlanch , I could not make the action as null, I have assigned the action name to an empty string and updated the unit test in adherence . Can you please have a look ?

Hi @CodeBlanch , Can you please review this PR ?

[CodeBlanch]

@mconnew Hey I could use some help on this if you have any free cycles.

So our telemetry for WCF calls does this...

• Span name: {Action}
• Tag rpc.service: {ContractName}
• Tag rpc.method: {OperationName}

How we find ContractName + OperationName is we pre-build a lookup dictionary:

opentelemetry-dotnet-contrib/src/OpenTelemetry.Contrib.Instrumentation.Wcf/TelemetryEndpointBehavior.cs

Lines 67 to 73 in 9373eb5

	foreach (var clientOperation in clientRuntime.ClientOperations)
	{
	actionMappings[clientOperation.Action] = new ActionMetadata
	{
	ContractName = $"{clientRuntime.ContractNamespace}{clientRuntime.ContractName}",
	OperationName = clientOperation.Name,
	};

When we have an Action to use for the lookup this all seems to work great.

Now the user is reporting a contract without action:

    [ServiceContract(Namespace = "http://opentelemetry.io/", Name = "Service", SessionMode = SessionMode.Allowed)]
    public interface IServiceContract
    {
        [OperationContract]
        Task<ServiceResponse> ExecuteAsync(ServiceRequest request);

        [OperationContract(Action = "")]
        Task<ServiceResponse> ExecuteWithEmptyActionNameAsync(ServiceRequest request);
    }

That sort of accidentally still works because the empty action still ends up on the lookup. But if contract has multiple empty actions...

    [ServiceContract(Namespace = "http://opentelemetry.io/", Name = "Service", SessionMode = SessionMode.Allowed)]
    public interface IServiceContract
    {
        [OperationContract]
        Task<ServiceResponse> ExecuteAsync(ServiceRequest request);

        [OperationContract(Action = "")]
        Task<ServiceResponse> ExecuteWithEmptyActionNameAsync(ServiceRequest request);

        [OperationContract(Action = "")]
        Task<ServiceResponse> SomeOtherOperationWithEmptyActionNameAsync(ServiceRequest request);
    }

...then the telemetry is going to lie and always pick the first empty entry.

What I need is some way in...

public object BeforeSendRequest(ref Message request, IClientChannel channel)

...to get at the operation data.

I don't see anything public to help. It seems like there is private stuff like Message.BodyWriter.OperationFormatter I could possibly use reflectively, but hoping to avoid that.

Any ideas?

[mconnew]

@CodeBlanch, does an empty Action actually work for multiple operations? Here's the default code where WCF looks up which operation is being executed based on the incoming message, and as you can see it uses a dictionary with the action as the key. So two operations with the same action won't work by default. I took a deeper look at the code to see if there should be an exception on service startup, and it looks like there should be as ActionDemuxer is calling HybridDictionary.Add which throws on duplicates. This is only with the default behavior though.

There is another alternative how this might work. If an implementation of IDispatchOperationSelector has been provided in the application, then the operation name is resolved by calling IDispatchOperationSelector.SelectOperation(ref request). In this case it's the applications implementation of the SelectOperation method which works out which operation is being used. You can extend the current implementation to check if an instance of IDispatchOperationSelector has been provided, and if so use that to get the operation name from the Message.

The only way I am aware of where that you can have two operations with the same action without it throwing when you start the service is to use a custom IDispatchOperationSelector. If I'm wrong about this, let me know and I'll do some deeper investigation to work out what's happening.

[CodeBlanch]

@mconnew Thanks for the great info! I did a bit of testing. With two operations defined with Action = '' starting a server, I get an exception. Starting a client seems to work fine. I think in that case the logic is good and should cover the common case. I'll implement IDispatchOperationSelector as a separate PR.

[mconnew]

It's allowed on the client as you can have a Task based and synchronous version of the same operation in a contract. They must be equivalent though. I think it might even modify the name so they both have the same operation name. There's no ambiguity about what to do there. It's only on the server side that you can't have the ambiguity.

[tamilaban]

Thanks much @CodeBlanch and @mconnew .