Throw NotSupportedException when using SetErrorStatusOnException in Mono Runtime and Native AOT environment.

[Yun-Ting]

Mitigates #5358
Design discussion issue #

Changes

Throw NotSupportedException when using ExceptionProcessor in Mono Runtime and Native AOT environment.

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)

[reyang]

Is it possible to make this a build time error? I think that will help the developers to discover the issue earlier.

[MichalStrehovsky]

If throwing is the only possible course of action, maybe this should be "fixed" on the .NET runtime side.

RuntimeFeature.IsDynamicCodeSupported is not great for approximating this (as discussed in #5358) because we might have a configuration in the future that responds false to this while still supporting this API.

The only reason to add this check here is that it will consistently throw between F5 debugging (where we emulate IsDynamicCodeSupported == false even though the runtime supports GetExceptionPointers), and publish (that doesn't support GetExceptionPointers, and also responds false to IsDynamicCodeSupported).

I wonder if we should instead "break" this API during F5 debugging instead - same way we break Reflection.Emit or COM interop when PublishAot is set in the project.

Then OpenTelemetry doesn't need to overapproximate the conditions when GetExceptionPointers won't work.

@eerhardt @AaronRobinsonMSFT @jkotas @agocke any opinion?

[eerhardt]

I agree that it is a better experience to get an exception during F5 development when PublishAot is set in the project. So I'd be in favor of throwing from the .NET runtime if possible.

For my education, what would it take to actually support the API in native AOT? Or provide some other API that would allow opentelemetry to work on all platforms. (See #1853 and #1858 for the scenario that this code is trying to enable.)

[jkotas]

Suggested change

	if (RuntimeFeature.IsDynamicCodeSupported)
	bool getExceptionPointersSupported = true;
	try { Marshal.GetExceptionPointers(); } catch { getExceptionPointersSupported = false; }
	if (!getExceptionPointersSupported)
	throw new throw new NotSupportedException($"'{typeof(Marshal).FullName}.GetExceptionPointers' is not supported.");

Instead of calling RuntimeFeature.IsDynamicCodeSupported, can this just call the API to see whether it is supported?

Note that this API is also not supported in Mono (irrespective of whether IsDynamicCodeSupported is true or false), so it would cover the Mono.

[Yun-Ting]

what would it take to actually support the API in native AOT?

Yes, it should be possible to simulated the API in native AOT. The API would need to allocate a piece of memory with the same lifetime as the current exception in flight (if it was not previously allocated), fill it with gunk, and return the pointer to it. The "new" exception handling that is being enabled for CoreCLR and that is not internally based on SEH proves that it is possible.

(Also, as I have said earlier - introducing a new API that is explicitly designed for this scenario would be the proper solution.)

Thank you! I agree above is the proper way to handle the situation. Shall I log this feature request in the runtime repo?

[jkotas]

Shall I log this feature request in the runtime repo?

Yes, please.

[Yun-Ting]

I've filed a request to implement the API in runtime: dotnet/runtime#98878.
Chatted with @eerhardt, @jkotas and @reyang offline.

While the above API is being implemented, we may do the followings to avoid crashing the consumer's app:

1. Annotate SetErrorStatusOnException() with [RequiresDynamicCode] to serve as a compile time warning for the consumer. One caveat of this approach is IsDynamicCode does not really explain the nature of this issue and may further confuse the consumer.
2. Use try/catch to swallow the exception if users hit System.PlatformNotSupportedException upon calling Marshal.GetExceptionPointer(). Store the state in ExceptionProcessor() and dump the Message "Exception processor not supported" inside OnEnd(Activity activity).

Not sure which one is a better stopgap of this issue?
cc @MichalStrehovsky, @agocke

[Yun-Ting]

Discussed in today's SIG meeting.
I'll go with adding the compile time warning as was mentioned in 1. above.
Plus, guarding the extension method

opentelemetry-dotnet/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderSdk.cs

Line 144 in e7cbbbb

public TracerProviderBuilder SetErrorStatusOnException(bool enabled)

with a try/catch on Marshal.GetExceptionPointers to avoid adding useless Processor at runtime (if user ever decided to suppress the compile time AOT warning.)

[utpilla]

There is no need to add try/catch block. We just have to call Marshal.GetExceptionPointers() once in the ExceptionProcessor ctor and rethrow the exception with our custom message.

@CodeBlanch Just to confirm this is what you suggested, right?

[CodeBlanch]

I'm OK with either re-throwing or just silently failing & not adding the processor. Probably re-throwing is better to alert the users 🤔

[vishweshbankwar]

+1 on re-throwing the exception at start-up.

[jkotas]

if we should instead "break" this API during F5 debugging instead - same way we break Reflection.Emit or COM interop when PublishAot is set in the project.

I do not think that it is worth it. OpenTelemetry is not using for what the API is meant for. This API exists to support _exception_info C++ compiler intrinsic that is part of SEH.

Then OpenTelemetry doesn't need to overapproximate the conditions when GetExceptionPointers won't work.

I expect that OpenTelemetry would still want to do something to avoid crashing, so this would not help. Check my suggestion https://github.com/open-telemetry/opentelemetry-dotnet/pull/5374/files#r1497791686

what would it take to actually support the API in native AOT?

Yes, it should be possible to simulated the API in native AOT. The API would need to allocate a piece of memory with the same lifetime as the current exception in flight (if it was not previously allocated), fill it with gunk, and return the pointer to it. The "new" exception handling that is being enabled for CoreCLR and that is not internally based on SEH proves that it is possible.

(Also, as I have said earlier - introducing a new API that is explicitly designed for this scenario would be the proper solution.)

[jkotas]

GetExceptionPointers() seems to be used as a unique identifier of the exception in flight here. GetExceptionPointers() is not a unique identifier of the exception in flight. Sequence of two thrown and caught exceptions can have the same pointer returned from GetExceptionPointers() if (snapshot != pointers) check does not look reliable to me. It may be another reason to introduce a new API for this scenario.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.52%. Comparing base (6250307) to head (8db5d36).
Report is 109 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5374      +/-   ##
==========================================
+ Coverage   83.38%   84.52%   +1.14%     
==========================================
  Files         297      284      -13     
  Lines       12531    12105     -426     
==========================================
- Hits        10449    10232     -217     
+ Misses       2082     1873     -209     

Flag	Coverage Δ
unittests	?
unittests-Solution-Experimental	84.51% <100.00%> (?)
unittests-Solution-Stable	84.50% <100.00%> (?)

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

Files	Coverage Δ
...y/Trace/Builder/TracerProviderBuilderExtensions.cs	100.00% <ø> (ø)
src/OpenTelemetry/Trace/ExceptionProcessor.cs	100.00% <100.00%> (ø)

... and 57 files with indirect coverage changes