-
Notifications
You must be signed in to change notification settings - Fork 385
[Instrumentation.Wcf] Allow WCF services to record exceptions if RecordException is set
#2880
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
Changes from all commits
c80a454
70be6a6
75e6022
46659e6
5defa5f
d64ffa9
e5afc8f
823777c
35ef4e9
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 |
|---|---|---|
|
|
@@ -104,6 +104,11 @@ internal TelemetryDispatchMessageInspector(IDictionary<string, ActionMetadata> a | |
| } | ||
| } | ||
|
|
||
| if (WcfInstrumentationActivitySource.Options.RecordException) | ||
| { | ||
| OperationContext.Current?.Extensions.Add(new WcfOperationContext(activity)); | ||
| } | ||
|
Comment on lines
+107
to
+110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have been doing something similar, and yes, OperationContext.Current will be more reliable for this use than Activity.Current, because when the WCF dispatcher pauses an operation and resumes another operation, it restores only OperationContext and not the whole ExecutionContext. My implementation additionally has an ICallContextInitializer that finds the (already started) Activity from OperationContext.Current and sets it as Activity.Current. I added that because I suspected that throttling in the WCF dispatcher might mess up the AsyncLocal<T> states. However, I haven't yet tested whether that actually happens. This OpenTelemetry WCF instrumentation doesn't seem to have such an ICallContextInitializer. Perhaps one should be added; but this PR does not make the problem worse. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe you are correct that the throttles would mess up the AsyncLocal<T> state as it essentially maintains a list of continuations to run in order as throttles are released and the ExecutionContext is captured with context flow disabled. Definitely worth adding this as a follow up item. |
||
|
|
||
| if (textMapPropagator is not TraceContextPropagator) | ||
| { | ||
| Baggage.Current = ctx.Baggage; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| #if NETFRAMEWORK | ||
| using System.Diagnostics; | ||
| using System.ServiceModel; | ||
| using System.ServiceModel.Channels; | ||
| using System.ServiceModel.Dispatcher; | ||
|
|
||
| namespace OpenTelemetry.Instrumentation.Wcf.Implementation; | ||
|
|
||
| internal class TracingErrorHandler : IErrorHandler | ||
| { | ||
| public bool HandleError(Exception error) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| public void ProvideFault(Exception error, MessageVersion version, ref Message fault) | ||
| { | ||
| // By rights this should be in `HandleError` instead, which would keep it from | ||
| // interfering with the response to the client. | ||
| // However, by the time `HandleError` fires, the Activity has already been stopped | ||
| // so the error appears after the Activity has completed. Additionally, sometimes | ||
| // the context has already been disposed or otherwise lost, preventing association | ||
| // at all. | ||
| // Also it becomes very difficult to unit-test, because there is no easy `ErrorsHandled` | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is perfectly fine to do here. HandleError allows you to prevent the session/context/connection from being aborted, and ProvideFault enable you to provde a custom fault message. Neither one of them are designed specifically for what you're doing, so there's no "right" place to do this. HanndleError is no more the right place than ProvideFault, so if this method works better due to the activity being stopped, then it's the right method to use. |
||
| // event to listen for before checking to see whether the error was logged. | ||
|
|
||
| if (!WcfInstrumentationActivitySource.Options?.RecordException ?? false) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // OperationContext.Current *should* be reliable even in async calls at .NET 4.6.2+. | ||
| // In older versions it may not be. | ||
| var context = OperationContext.Current?.Extensions.Find<WcfOperationContext>(); | ||
| var activity = context?.Activity ?? WcfInstrumentationActivitySource.ActivitySource.StartActivity(WcfInstrumentationActivitySource.UnassociatedExceptionActivityName, ActivityKind.Internal); | ||
|
|
||
| activity?.AddException(error); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you need the null coalescing here? I presume StartActivity is guaranteed to return a non-null activity, which means There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's not. StartActivity returns null if the ActivitySource has no listeners or if no ActivityListener chooses to sample the activity. |
||
|
|
||
| if (activity != context?.Activity) | ||
| { | ||
| activity?.Stop(); | ||
| } | ||
| } | ||
| } | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| #if NETFRAMEWORK | ||
| using System.Diagnostics; | ||
| using System.ServiceModel; | ||
|
|
||
| namespace OpenTelemetry.Instrumentation.Wcf.Implementation; | ||
|
|
||
| internal class WcfOperationContext : IExtension<OperationContext> | ||
| { | ||
| public WcfOperationContext(Activity activity) | ||
| { | ||
| this.Activity = activity; | ||
| } | ||
|
|
||
| public Activity Activity { get; } | ||
|
|
||
| public void Attach(OperationContext owner) | ||
| { | ||
| } | ||
|
|
||
| public void Detach(OperationContext owner) | ||
| { | ||
| } | ||
| } | ||
| #endif |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -205,6 +205,114 @@ await client.ExecuteAsync( | |
| Assert.Empty(stoppedActivities); | ||
| } | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(true, true, true)] | ||
| [InlineData(true, true, false)] | ||
| [InlineData(true, false, true)] | ||
| [InlineData(false, true, true)] | ||
| [InlineData(true, false, false)] | ||
| [InlineData(false, false, true)] | ||
| [InlineData(false, true, false)] | ||
| [InlineData(false, false, false)] | ||
| public async Task RecordExceptionTest( | ||
| bool recordException, | ||
| bool triggerException, | ||
| bool runAsync) | ||
| { | ||
| List<Activity> stoppedActivities = []; | ||
| List<Activity> startedActivities = []; | ||
|
|
||
| List<Exception> recordedExceptions = []; | ||
| using var activityListener = new ActivityListener | ||
| { | ||
| ShouldListenTo = activitySource => true, | ||
| ActivityStarted = startedActivities.Add, | ||
| ActivityStopped = stoppedActivities.Add, | ||
| }; | ||
|
|
||
| activityListener.ExceptionRecorder += (Activity activity, Exception ex, ref TagList tags) => | ||
| { | ||
| recordedExceptions.Add(ex); | ||
| }; | ||
|
Comment on lines
+234
to
+237
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered whether this ExceptionRecorder should check that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WCF in .NET Framework isn't adding anything like this. We were going to initially, but the goal posts kept moving on what telemetry was going to look like, and they keep changing in small subtle ways (evolving), so adding telemetry externally via this package is going to be the best place for it to exist for a long time. And the more time passes, the less interest there is in adding anything to .NET Framework as all the focus is on .NET. |
||
|
|
||
| ActivitySource.AddActivityListener(activityListener); | ||
|
|
||
| var tracerProvider = Sdk.CreateTracerProviderBuilder() | ||
| .AddWcfInstrumentation(options => | ||
| { | ||
| options.RecordException = recordException; | ||
| }) | ||
| .Build(); | ||
|
|
||
| var client = new ServiceClient( | ||
| new NetTcpBinding(), | ||
| new EndpointAddress(new Uri(this.serviceBaseUri, "/Service"))); | ||
| try | ||
| { | ||
| if (triggerException) | ||
| { | ||
| if (runAsync) | ||
| { | ||
| await client.ErrorAsync(); | ||
| } | ||
| else | ||
| { | ||
| client.ErrorSynchronous(); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| if (runAsync) | ||
| { | ||
| await client.ExecuteAsync( | ||
| new ServiceRequest( | ||
| payload: "Hello Open Telemetry!")); | ||
| } | ||
| else | ||
| { | ||
| client.ExecuteSynchronous( | ||
| new ServiceRequest( | ||
| payload: "Hello Open Telemetry!")); | ||
| } | ||
| } | ||
| } | ||
| catch (Exception) | ||
| { | ||
| } | ||
| finally | ||
| { | ||
| startedActivities[0].AddTag(nameof(recordException), recordException); | ||
| startedActivities[0].AddTag(nameof(triggerException), triggerException); | ||
| startedActivities[0].AddTag(nameof(runAsync), runAsync); | ||
|
|
||
| if (client.State == CommunicationState.Faulted) | ||
| { | ||
| client.Abort(); | ||
| } | ||
| else | ||
| { | ||
| client.Close(); | ||
| } | ||
|
|
||
| tracerProvider?.Shutdown(); | ||
| tracerProvider?.Dispose(); | ||
|
|
||
| WcfInstrumentationActivitySource.Options = null; | ||
| } | ||
|
|
||
| Assert.NotEmpty(stoppedActivities); | ||
| var activity = Assert.Single(stoppedActivities); | ||
|
|
||
| if (recordException && triggerException) | ||
| { | ||
| Assert.Collection(recordedExceptions, e => Assert.IsType<Exception>(e)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, this verifies the type of the exception is exactly Exception as thrown from |
||
| } | ||
| else | ||
| { | ||
| Assert.Empty(recordedExceptions); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #endif | ||
Uh oh!
There was an error while loading. Please reload this page.