Skip to content
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

Add SetErrorStatusOnException option to TracerProviderSdk #1858

Merged
merged 24 commits into from
Mar 6, 2021

Conversation

reyang
Copy link
Member

@reyang reyang commented Feb 28, 2021

Related to #1853.

Changes

  • Added TracerProviderOptions to allow configurations while creating the tracer provider. Inspired by @CodeBlanch per offline discussion.
  • Introduced ExceptionProcessor based on the idea from Automatically detect exceptions and set Activity status #1853.
    • I made this a processor, later we can add more features to it (by supporting more options/flags) - e.g. extract the SEH content and call activity.SetTag.
    • An alternative way is to put the logic inside TraceProviderSdk but I think it will be too bulky (and people who don't use this will pay for an extra if-condition check).

For significant contributions please make sure you have completed the following items:

@reyang reyang requested a review from a team February 28, 2021 02:16
@codecov
Copy link

codecov bot commented Feb 28, 2021

Codecov Report

Merging #1858 (c7b115f) into main (cb066cb) will increase coverage by 0.28%.
The diff coverage is 97.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1858      +/-   ##
==========================================
+ Coverage   83.77%   84.06%   +0.28%     
==========================================
  Files         187      189       +2     
  Lines        5967     6036      +69     
==========================================
+ Hits         4999     5074      +75     
+ Misses        968      962       -6     
Impacted Files Coverage Δ
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 71.42% <ø> (ø)
...rc/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs 88.46% <88.88%> (-2.28%) ⬇️
src/OpenTelemetry/Trace/ExceptionProcessor.cs 90.90% <90.90%> (ø)
src/OpenTelemetry/Trace/TracerProviderSdk.cs 95.16% <98.64%> (+4.38%) ⬆️
...metryProtocol/Implementation/ActivityExtensions.cs 86.89% <100.00%> (+0.05%) ⬆️
...ry.Instrumentation.AspNet/AspNetInstrumentation.cs 100.00% <100.00%> (ø)
...umentation.AspNet/Implementation/HttpInListener.cs 89.10% <100.00%> (ø)
...entation.AspNet/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
...umentation.AspNetCore/AspNetCoreInstrumentation.cs 100.00% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs 86.56% <100.00%> (ø)
... and 20 more

@reyang reyang changed the title Add SetErrorStatusOnUnhandledException option to TracerProviderSdk Add SetErrorStatusOnException option to TracerProviderSdk Mar 1, 2021
@reyang reyang linked an issue Mar 1, 2021 that may be closed by this pull request
{
if (this.fnGetExceptionPointers() != IntPtr.Zero)
{
activity.SetStatus(Status.Error);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to get the type, message, and/or stack from the IntPtr returned from GetExceptionPointers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is possible by traveling through the SEH object. This is CPU/OS/platform dependent and not well documented.
https://bytepointer.com/resources/pietrek_crash_course_depths_of_win32_seh.htm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks petty intense! Not sure if it is worth attempting.

Why I bring it up:

I was just thinking about the applications I have instrumented with OpenTelemetry .NET so far. There are really only a few spots I've had the need to create spans manually. Almost exclusively around queues/topics. In those cases, I'm doing what the example has, with one exception...

catch (Exception ex)
{
   activity.SetStatus(Status.Error.WithDescription(ex.Message));
   throw;
}

...instead of...

catch (Exception)
{
   activity.SetStatus(Status.Error);
   throw;
}

So, commenting from the perspective of a user of the library, since there's only a handful of spots where I have the try/catch, and the message/description is super useful, I'll probably continue doing the try/catch instead of using the processor/option. But if the processor could grab the message too, that would be something.

Copy link
Member Author

@reyang reyang Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to cover it in a follow up PR.
One heads up, the exceptions we captured here could be any SEH exception - e.g. C++ exception, OS exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using #1874 to explore how deep should we go.

@@ -0,0 +1,71 @@
# Exception Handling
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ this guide 😄

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ThomsonTan ThomsonTan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!


A complete example can be found [here](./Program.cs).

Note: this feature is platform dependent as it relies on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the doc it looks like its support in all cases except in .NET Core 2.1.
Do you know if it works in windows only?
While we can generally redirect users to official docs, we can list any obvious things right here in this doc . for example, if this only works in Windows, we can mention it here (along with link to official doc)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works on Mac (x64) and Linux (x86 VM) as I've tested locally (I also created issue #1859 to cover ARM64).

@reyang reyang closed this Mar 5, 2021
@reyang reyang reopened this Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically detect exceptions and set Activity status
4 participants