From 954b81cb72ea1b715d4f54fcf860fd38471ae4b0 Mon Sep 17 00:00:00 2001 From: ievgen-platonov <72199261+ievgen-platonov@users.noreply.github.com> Date: Thu, 17 Mar 2022 19:04:01 +0200 Subject: [PATCH] #229: NullReferenceException in TelemetryDispatchMessageInspector.BeforeSendReply when operation is OneWay (#237) * Update TelemetryDispatchMessageInspector.cs #229: Check if reply is not null * #229: Added TelemetryDispatchMessageInspectorForOneWayOperationsTests * #229: Rename parameters * #229: Added new "1.0.0-rc6" version to CHANGELOG.md * #229: Fix text formatting * #229: Fix CHANGELOG formatting. Added extra asserts to the test * #229: Fix formatting * #229: Fix formatting in GHANGELOG.md Co-authored-by: iepl Co-authored-by: Ievgen Platonov <77076677+iepl@users.noreply.github.com> --- .../CHANGELOG.md | 6 + .../TelemetryDispatchMessageInspector.cs | 2 +- ...InspectorForOneWayOperationsTests.netfx.cs | 158 ++++++++++++++++++ .../Tools/ErrorHandler.netfx.cs | 48 ++++++ .../ErrorHandlerServiceBehavior.netfx.cs | 55 ++++++ .../WCF/IServiceContract.cs | 3 + .../WCF/Service.netfx.cs | 4 + .../WCF/ServiceClient.cs | 3 + 8 files changed, 278 insertions(+), 1 deletion(-) create mode 100644 test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/TelemetryDispatchMessageInspectorForOneWayOperationsTests.netfx.cs create mode 100644 test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/Tools/ErrorHandler.netfx.cs create mode 100644 test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/Tools/ErrorHandlerServiceBehavior.netfx.cs diff --git a/src/OpenTelemetry.Contrib.Instrumentation.Wcf/CHANGELOG.md b/src/OpenTelemetry.Contrib.Instrumentation.Wcf/CHANGELOG.md index 4c3495aadf9..52eeb96fa78 100644 --- a/src/OpenTelemetry.Contrib.Instrumentation.Wcf/CHANGELOG.md +++ b/src/OpenTelemetry.Contrib.Instrumentation.Wcf/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## 1.0.0-rc6 + +* Fixed a `NullReferenceException` in + `TelemetryDispatchMessageInspector.BeforeSendReply` when operation is OneWay + ([#237](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/237)) + ## 1.0.0-rc5 * Fixed an `ArgumentNullException` setting `Activity`.`DisplayName` when diff --git a/src/OpenTelemetry.Contrib.Instrumentation.Wcf/TelemetryDispatchMessageInspector.cs b/src/OpenTelemetry.Contrib.Instrumentation.Wcf/TelemetryDispatchMessageInspector.cs index b9d6b9768a4..7f03eafe66f 100644 --- a/src/OpenTelemetry.Contrib.Instrumentation.Wcf/TelemetryDispatchMessageInspector.cs +++ b/src/OpenTelemetry.Contrib.Instrumentation.Wcf/TelemetryDispatchMessageInspector.cs @@ -131,7 +131,7 @@ public void BeforeSendReply(ref Message reply, object correlationState) { if (correlationState is Activity activity) { - if (activity.IsAllDataRequested) + if (activity.IsAllDataRequested && reply != null) { if (reply.IsFault) { diff --git a/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/TelemetryDispatchMessageInspectorForOneWayOperationsTests.netfx.cs b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/TelemetryDispatchMessageInspectorForOneWayOperationsTests.netfx.cs new file mode 100644 index 00000000000..69f7c7a9c02 --- /dev/null +++ b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/TelemetryDispatchMessageInspectorForOneWayOperationsTests.netfx.cs @@ -0,0 +1,158 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#if NETFRAMEWORK +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; +using System.ServiceModel; +using System.Threading; +using OpenTelemetry.Contrib.Instrumentation.Wcf.Tests.Tools; +using OpenTelemetry.Trace; +using Xunit; +using Xunit.Abstractions; + +namespace OpenTelemetry.Contrib.Instrumentation.Wcf.Tests +{ + [Collection("WCF")] + public class TelemetryDispatchMessageInspectorForOneWayOperationsTests : IDisposable + { + private readonly ITestOutputHelper output; + private readonly Uri serviceBaseUri; + private readonly ServiceHost serviceHost; + + private readonly EventWaitHandle thrownExceptionsHandle = new EventWaitHandle(false, EventResetMode.ManualReset); + private readonly List thrownExceptions = new List(); + + public TelemetryDispatchMessageInspectorForOneWayOperationsTests(ITestOutputHelper outputHelper) + { + this.output = outputHelper; + + Random random = new Random(); + var retryCount = 5; + while (retryCount > 0) + { + try + { + this.serviceBaseUri = new Uri($"net.tcp://localhost:{random.Next(2000, 5000)}/"); + this.serviceHost = new ServiceHost(new Service(), this.serviceBaseUri); + + var endpoint = this.serviceHost.AddServiceEndpoint( + typeof(IServiceContract), + new NetTcpBinding(), + "/Service"); + endpoint.Behaviors.Add(new TelemetryEndpointBehavior()); + + this.serviceHost.Description.Behaviors.Add( + new ErrorHandlerServiceBehavior(this.thrownExceptionsHandle, ex => this.thrownExceptions.Add(ex))); + + this.serviceHost.Open(); + + break; + } + catch (Exception ex) + { + this.output.WriteLine(ex.ToString()); + if (this.serviceHost.State == CommunicationState.Faulted) + { + this.serviceHost.Abort(); + } + else + { + this.serviceHost.Close(); + } + + this.serviceHost = null; + retryCount--; + } + } + + if (this.serviceHost == null) + { + throw new InvalidOperationException("ServiceHost could not be started."); + } + } + + public void Dispose() + { + this.serviceHost?.Close(); + this.thrownExceptionsHandle?.Dispose(); + } + + [Fact] + public void IncomingRequestOneWayOperationInstrumentationTest() + { + List stoppedActivities = new List(); + + using ActivityListener activityListener = new ActivityListener + { + ShouldListenTo = activitySource => true, + ActivityStopped = activity => stoppedActivities.Add(activity), + }; + + ActivitySource.AddActivityListener(activityListener); + TracerProvider tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddWcfInstrumentation() + .Build(); + + ServiceClient client = new ServiceClient( + new NetTcpBinding(), + new EndpointAddress(new Uri(this.serviceBaseUri, "/Service"))); + + try + { + client.ExecuteWithOneWay(new ServiceRequest()); + this.thrownExceptionsHandle.WaitOne(3000); + } + finally + { + if (client.State == CommunicationState.Faulted) + { + client.Abort(); + } + else + { + client.Close(); + } + + tracerProvider?.Shutdown(); + tracerProvider?.Dispose(); + + WcfInstrumentationActivitySource.Options = null; + } + + // Assert + Assert.Empty(this.thrownExceptions); + + Assert.NotEmpty(stoppedActivities); + Assert.Single(stoppedActivities); + + Activity activity = stoppedActivities[0]; + Assert.Equal("http://opentelemetry.io/Service/ExecuteWithOneWay", activity.DisplayName); + Assert.Equal("ExecuteWithOneWay", activity.TagObjects.FirstOrDefault(t => t.Key == WcfInstrumentationConstants.RpcMethodTag).Value); + Assert.DoesNotContain(activity.TagObjects, t => t.Key == WcfInstrumentationConstants.SoapReplyActionTag); + + Assert.Equal(WcfInstrumentationActivitySource.IncomingRequestActivityName, activity.OperationName); + Assert.Equal(WcfInstrumentationConstants.WcfSystemValue, activity.TagObjects.FirstOrDefault(t => t.Key == WcfInstrumentationConstants.RpcSystemTag).Value); + Assert.Equal("http://opentelemetry.io/Service", activity.TagObjects.FirstOrDefault(t => t.Key == WcfInstrumentationConstants.RpcServiceTag).Value); + Assert.Equal(this.serviceBaseUri.Host, activity.TagObjects.FirstOrDefault(t => t.Key == WcfInstrumentationConstants.NetHostNameTag).Value); + Assert.Equal(this.serviceBaseUri.Port, activity.TagObjects.FirstOrDefault(t => t.Key == WcfInstrumentationConstants.NetHostPortTag).Value); + Assert.Equal("net.tcp", activity.TagObjects.FirstOrDefault(t => t.Key == WcfInstrumentationConstants.WcfChannelSchemeTag).Value); + Assert.Equal("/Service", activity.TagObjects.FirstOrDefault(t => t.Key == WcfInstrumentationConstants.WcfChannelPathTag).Value); + } + } +} +#endif diff --git a/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/Tools/ErrorHandler.netfx.cs b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/Tools/ErrorHandler.netfx.cs new file mode 100644 index 00000000000..58961dbf62b --- /dev/null +++ b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/Tools/ErrorHandler.netfx.cs @@ -0,0 +1,48 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#if NETFRAMEWORK +using System; +using System.ServiceModel.Channels; +using System.ServiceModel.Dispatcher; +using System.Threading; + +namespace OpenTelemetry.Contrib.Instrumentation.Wcf.Tests.Tools +{ + internal class ErrorHandler : IErrorHandler + { + private readonly EventWaitHandle handle; + private readonly Action log; + + public ErrorHandler(EventWaitHandle handle, Action log) + { + this.handle = handle; + this.log = log; + } + + public bool HandleError(Exception error) + { + this.log(error); + this.handle.Set(); + + return true; + } + + public void ProvideFault(Exception error, MessageVersion version, ref Message fault) + { + } + } +} +#endif diff --git a/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/Tools/ErrorHandlerServiceBehavior.netfx.cs b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/Tools/ErrorHandlerServiceBehavior.netfx.cs new file mode 100644 index 00000000000..0dcccaf9a31 --- /dev/null +++ b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/Tools/ErrorHandlerServiceBehavior.netfx.cs @@ -0,0 +1,55 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#if NETFRAMEWORK +using System; +using System.Collections.ObjectModel; +using System.ServiceModel; +using System.ServiceModel.Channels; +using System.ServiceModel.Description; +using System.ServiceModel.Dispatcher; +using System.Threading; + +namespace OpenTelemetry.Contrib.Instrumentation.Wcf.Tests.Tools +{ + internal class ErrorHandlerServiceBehavior : IServiceBehavior + { + private readonly EventWaitHandle handle; + private readonly Action action; + + public ErrorHandlerServiceBehavior(EventWaitHandle handle, Action action) + { + this.handle = handle; + this.action = action; + } + + public void AddBindingParameters(ServiceDescription serviceDescription, ServiceHostBase serviceHostBase, Collection endpoints, BindingParameterCollection bindingParameters) + { + } + + public void ApplyDispatchBehavior(ServiceDescription serviceDescription, ServiceHostBase serviceHostBase) + { + foreach (ChannelDispatcher dispatcher in serviceHostBase.ChannelDispatchers) + { + dispatcher.ErrorHandlers.Add(new ErrorHandler(this.handle, this.action)); + } + } + + public void Validate(ServiceDescription serviceDescription, ServiceHostBase serviceHostBase) + { + } + } +} +#endif diff --git a/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/IServiceContract.cs b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/IServiceContract.cs index d4358f2b6db..49746d21783 100644 --- a/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/IServiceContract.cs +++ b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/IServiceContract.cs @@ -27,5 +27,8 @@ public interface IServiceContract [OperationContract(Action = "")] Task ExecuteWithEmptyActionNameAsync(ServiceRequest request); + + [OperationContract(IsOneWay = true)] + void ExecuteWithOneWay(ServiceRequest request); } } diff --git a/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/Service.netfx.cs b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/Service.netfx.cs index aa75e5e8bf9..0f8aa6c9f54 100644 --- a/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/Service.netfx.cs +++ b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/Service.netfx.cs @@ -44,6 +44,10 @@ public Task ExecuteWithEmptyActionNameAsync(ServiceRequest requ Payload = $"RSP: {request.Payload}", }); } + + public void ExecuteWithOneWay(ServiceRequest request) + { + } } } #endif diff --git a/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/ServiceClient.cs b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/ServiceClient.cs index 98792bb3c12..a3af9145112 100644 --- a/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/ServiceClient.cs +++ b/test/OpenTelemetry.Contrib.Instrumentation.Wcf.Tests/WCF/ServiceClient.cs @@ -32,5 +32,8 @@ public Task ExecuteAsync(ServiceRequest request) public Task ExecuteWithEmptyActionNameAsync(ServiceRequest request) => this.Channel.ExecuteWithEmptyActionNameAsync(request); + + public void ExecuteWithOneWay(ServiceRequest request) + => this.Channel.ExecuteWithOneWay(request); } }