[Instrumentation.Wcf] Allow WCF services to record exceptions if `RecordException` is set by AngleOSaxon · Pull Request #2880 · open-telemetry/opentelemetry-dotnet-contrib

AngleOSaxon · 2025-07-05T19:55:45Z

Implements #2878

Changes

Adds an IErrorHandler implementation (TracingErrorHandler) that gets added to the ChannelDispatcher instances for any instrumented services. If the RecordExceptions option is set, it associates the current Activity with OperationContext.Current during the initial message inspection. When an exception is triggered, the TracingErrorHandler loads the Activity and adds the exception to it.

Although the documentation specifically notes that the HandleError method should be used to perform activities such as logging, I'm performing the logging in the ProvideFault method. This is being done for reasons of timing--ProvideFault is called before the message is returned to the client and therefore before the Activity is stopped in TelemetryDispatchMessageInspector.BeforeSendReply. This allows the exception to be properly placed inside the recorded span, prevents it from being attached to an already completed Activity, and ensures the OperationContext with the relevant information still exists.

It also simplifies the testing considerably, because I have not been able to find an event that only fires after all HandleError implementations have been called, so I'm forced to simply rely on predefined waits to see if the error is recorded.

As far as I am aware, the biggest potential issue with using ProvideFault is it will block completing the request and sending the response to the client. I don't expect that to be an issue here, but it will rely on the good behavior of the ActivityListener.ExceptionRecorder implementations.

Merge requirement checklist

CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

linux-foundation-easycla · 2025-07-05T19:55:50Z

The committers listed above are authorized under a signed CLA.

✅ login: AngleOSaxon / name: Andrew Bikle (c80a454, 70be6a6, 75e6022, d64ffa9, 5defa5f, 46659e6, e5afc8f, 823777c)
✅ login: Kielek / name: Piotr Kiełkowicz (35ef4e9)

github-actions · 2025-07-13T03:43:38Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

KalleOlaviNiemitalo · 2025-07-13T06:49:36Z

+
+        // 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.FirstOrDefault(item => item is WcfOperationContext) as WcfOperationContext;


IExtensionCollection<T>.Find<E> would be more concise.

Kielek

@mconnew, is there any chance that you find some time to review general concept in this PR?

KalleOlaviNiemitalo · 2025-07-14T06:17:10Z

+        activityListener.ExceptionRecorder += (Activity activity, Exception ex, ref TagList tags) =>
+        {
+            recordedExceptions.Add(ex);
+        };


I considered whether this ExceptionRecorder should check that activity.Source.Name is as expected. Just to verify that the exception is being recorded by the OpenTelemetry WCF instrumentation rather than any WCF built-in feature. However, if WCF somehow got such a feature, then the recordException: false test cases would fail. So it's not necessary to check the ActivitySource here.

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.

KalleOlaviNiemitalo · 2025-07-14T06:29:50Z

+
+        if (recordException && triggerException)
+        {
+            Assert.Collection(recordedExceptions, e => Assert.IsType<Exception>(e));


OK, this verifies the type of the exception is exactly Exception as thrown from test/OpenTelemetry.Instrumentation.Wcf.Tests/WCF/Service.netfx.cs, not translated to any FaultException.

KalleOlaviNiemitalo · 2025-07-14T06:49:17Z

+            if (WcfInstrumentationActivitySource.Options.RecordException)
+            {
+                OperationContext.Current?.Extensions.Add(new WcfOperationContext(activity));
+            }


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.

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.

Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>

Co-authored-by: Matt Hensley <130569+matt-hensley@users.noreply.github.com>

KalleOlaviNiemitalo · 2025-07-24T08:34:04Z

Should mention the feature around here, rather than say it "is available only on the client side":

opentelemetry-dotnet-contrib/src/OpenTelemetry.Instrumentation.Wcf/README.md

Lines 232 to 237 in 4448613

    
           ### RecordException 
        
           This instrumentation automatically sets Activity Status to Error if an unhandled 
        
           exception is thrown. Additionally, `RecordException` feature may be turned on, 
        
           to store the exception to the Activity itself as ActivityEvent. `RecordException` 
        
           is available only on the client side.

github-actions · 2025-08-01T03:48:24Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

AngleOSaxon · 2025-08-02T16:05:06Z

Should mention the feature around here, rather than say it "is available only on the client side":

opentelemetry-dotnet-contrib/src/OpenTelemetry.Instrumentation.Wcf/README.md

Lines 232 to 237 in 4448613

### RecordException

This instrumentation automatically sets Activity Status to Error if an unhandled

exception is thrown. Additionally, `RecordException` feature may be turned on,

to store the exception to the Activity itself as ActivityEvent. `RecordException`

is available only on the client side.

Done

mconnew · 2025-08-03T03:43:37Z

@kiel, sorry I missed the mention earlier. I'll take a look this week.

github-actions · 2025-08-11T03:45:43Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

mconnew · 2025-08-11T23:18:01Z

+        // 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`


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.

mconnew · 2025-08-11T23:27:02Z

+        var context = OperationContext.Current?.Extensions.Find<WcfOperationContext>();
+        var activity = context?.Activity ?? WcfInstrumentationActivitySource.ActivitySource.StartActivity(WcfInstrumentationActivitySource.UnassociatedExceptionActivityName, ActivityKind.Internal);
+
+        activity?.AddException(error);


Do you need the null coalescing here? I presume StartActivity is guaranteed to return a non-null activity, which means activity can't be null here. Same with the call to Stop below.

I presume StartActivity is guaranteed to return a non-null activity

It's not. StartActivity returns null if the ActivitySource has no listeners or if no ActivityListener chooses to sample the activity.

mconnew · 2025-08-11T23:30:23Z

I've approved the changes, but I don't have write access to this repo, so it's still blocked.

Kielek

Code changes LGTM..
@mconnew is domain expert here, so I am approving based on his judgement.

codecov · 2025-08-18T07:59:38Z

Codecov Report

❌ Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.76%. Comparing base (b1767d6) to head (35ef4e9).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...entation.Wcf/Implementation/TracingErrorHandler.cs	88.88%	1 Missing ⚠️
...entation.Wcf/Implementation/WcfOperationContext.cs	83.33%	1 Missing ⚠️
...entation.Wcf/TelemetryContractBehaviorAttribute.cs	0.00%	1 Missing ⚠️
...ry.Instrumentation.Wcf/TelemetryServiceBehavior.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2880      +/-   ##
==========================================
+ Coverage   69.59%   69.76%   +0.16%     
==========================================
  Files         396      388       -8     
  Lines       15906    15876      -30     
==========================================
+ Hits        11070    11076       +6     
+ Misses       4836     4800      -36

Flag	Coverage Δ
unittests-Instrumentation.Cassandra	`?`
unittests-Instrumentation.Wcf	`78.95% <80.95%> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...mplementation/TelemetryDispatchMessageInspector.cs	`90.76% <100.00%> (+1.88%)`	⬆️
...Implementation/WcfInstrumentationActivitySource.cs	`100.00% <100.00%> (ø)`
...y.Instrumentation.Wcf/TelemetryEndpointBehavior.cs	`91.42% <100.00%> (+0.25%)`	⬆️
...entation.Wcf/Implementation/TracingErrorHandler.cs	`88.88% <88.88%> (ø)`
...entation.Wcf/Implementation/WcfOperationContext.cs	`83.33% <83.33%> (ø)`
...entation.Wcf/TelemetryContractBehaviorAttribute.cs	`0.00% <0.00%> (ø)`
...ry.Instrumentation.Wcf/TelemetryServiceBehavior.cs	`0.00% <0.00%> (ø)`

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

AngleOSaxon requested a review from a team as a code owner July 5, 2025 19:55

github-actions Bot added the comp:instrumentation.wcf Things related to OpenTelemetry.Instrumentation.Wcf label Jul 5, 2025

github-actions Bot added the Stale label Jul 13, 2025

KalleOlaviNiemitalo reviewed Jul 13, 2025

View reviewed changes

github-actions Bot removed the Stale label Jul 14, 2025

Kielek reviewed Jul 14, 2025

View reviewed changes

Comment thread src/OpenTelemetry.Instrumentation.Wcf/Implementation/WcfOperationContext.cs

Comment thread src/OpenTelemetry.Instrumentation.Wcf/CHANGELOG.md

KalleOlaviNiemitalo reviewed Jul 14, 2025

View reviewed changes

AngleOSaxon force-pushed the wcf-error-errorhandler branch from d4421c8 to 7c66aaa Compare July 14, 2025 21:17

AngleOSaxon and others added 4 commits July 20, 2025 13:15

Allow WCF services to record exceptions if RecordException is set

c80a454

Simplify finding extension

70be6a6

Conditionally include custom OperationContext

75e6022

Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>

Reference PR in changelog

46659e6

Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>

AngleOSaxon force-pushed the wcf-error-errorhandler branch from 7c66aaa to 46659e6 Compare July 20, 2025 17:15

matt-hensley reviewed Jul 22, 2025

View reviewed changes

Comment thread test/OpenTelemetry.Instrumentation.Wcf.Tests/TelemetryDispatchMessageInspectorTests.netfx.cs Outdated

AngleOSaxon and others added 2 commits July 23, 2025 20:32

Simplify check and assignment

5defa5f

Co-authored-by: Matt Hensley <130569+matt-hensley@users.noreply.github.com>

Merge branch 'main' into wcf-error-errorhandler

d64ffa9

github-actions Bot added the Stale label Aug 1, 2025

AngleOSaxon added 2 commits August 2, 2025 12:04

Update README.md to reference server-side functionality

e5afc8f

Merge branch 'main' into wcf-error-errorhandler

823777c

github-actions Bot removed the Stale label Aug 3, 2025

github-actions Bot added the Stale label Aug 11, 2025

mconnew reviewed Aug 11, 2025

View reviewed changes

mconnew approved these changes Aug 11, 2025

View reviewed changes

github-actions Bot removed the Stale label Aug 12, 2025

Kielek changed the title ~~Allow WCF services to record exceptions if RecordException is set~~ [Instrumentation.Wcf] Allow WCF services to record exceptions if RecordException is set Aug 18, 2025

Merge branch 'main' into wcf-error-errorhandler

35ef4e9

Kielek approved these changes Aug 18, 2025

View reviewed changes

Kielek merged commit 6ddcd9a into open-telemetry:main Aug 18, 2025
63 checks passed

Conversation

AngleOSaxon commented Jul 5, 2025

Changes

Merge requirement checklist

Uh oh!

linux-foundation-easycla Bot commented Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kielek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KalleOlaviNiemitalo commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 1, 2025

Uh oh!

AngleOSaxon commented Aug 2, 2025

Uh oh!

mconnew commented Aug 3, 2025

Uh oh!

github-actions Bot commented Aug 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mconnew commented Aug 11, 2025

Uh oh!

Kielek left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

linux-foundation-easycla Bot commented Jul 5, 2025 •

edited

Loading

KalleOlaviNiemitalo commented Jul 24, 2025 •

edited

Loading

codecov Bot commented Aug 18, 2025 •

edited

Loading