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

Fix ASP.NET Core ExceptionFilter prevents recording exception #3475

Merged
merged 14 commits into from
Aug 24, 2022

Conversation

al-mac
Copy link
Contributor

@al-mac al-mac commented Jul 22, 2022

Fixes #2853

ASP.NET Core exception filters fire events before and after filter execution.

Before:
https://github.com/dotnet/aspnetcore/blob/36df8e5dc487e27cb85674ef3f144dca07f05c20/src/Mvc/Mvc.Core/src/MvcCoreDiagnosticListenerExtensions.cs#L396-L402

After:
https://github.com/dotnet/aspnetcore/blob/c85baf8db0c72ae8e68643029d514b2e737c9fae/src/Mvc/Mvc.Core/src/MvcCoreDiagnosticListenerExtensions.cs#L422-L434

The ASP.NET Core instrumentation's OnException event is invoked when these events fire. The payload of the event differs from that of the regular exception path and precludes the exception from being recorded (if enabled) and the span status from being set.

Changes

Please provide a brief description of the changes here.

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

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@al-mac al-mac marked this pull request as ready for review July 22, 2022 20:08
@al-mac al-mac requested a review from a team July 22, 2022 20:08
@alanwest
Copy link
Member

A few small CI failures to resolve

Static analysis enforces style guidelines. You should be able to produce this locally, but might need to do a release build

D:\a\opentelemetry-dotnet\opentelemetry-dotnet\test\OpenTelemetry.Instrumentation.AspNetCore.Tests\BasicTests.cs(665,29): error SA1204: Static members should appear before non-static members [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\test\OpenTelemetry.Instrumentation.AspNetCore.Tests\OpenTelemetry.Instrumentation.AspNetCore.Tests.csproj]

We use markdownlint to enforce style in markdown files. You could run this locally too following instructions here, but in your case it looks you just need to add a newline after a header.

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md:3 MD022/blanks-around-headings/blanks-around-headers Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "## Unreleased"]

@al-mac
Copy link
Contributor Author

al-mac commented Jul 25, 2022

Fixed! Sorry about that.

@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #3475 (dfd3202) into main (968dc52) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3475      +/-   ##
==========================================
+ Coverage   87.26%   87.29%   +0.03%     
==========================================
  Files         277      277              
  Lines        9938     9944       +6     
==========================================
+ Hits         8672     8681       +9     
+ Misses       1266     1263       -3     
Impacted Files Coverage Δ
...DiagnosticSourceInstrumentation/PropertyFetcher.cs 92.50% <100.00%> (-4.56%) ⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs 75.00% <0.00%> (-20.00%) ⬇️
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs 72.72% <0.00%> (-13.64%) ⬇️
src/OpenTelemetry/BatchExportProcessor.cs 82.24% <0.00%> (-1.87%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 96.87% <0.00%> (-0.79%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 96.15% <0.00%> (+1.09%) ⬆️
...tation.AspNetCore/Implementation/HttpInListener.cs 91.41% <0.00%> (+1.22%) ⬆️
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 88.88% <0.00%> (+2.22%) ⬆️
...tpListener/Internal/PrometheusCollectionManager.cs 80.48% <0.00%> (+2.43%) ⬆️
... and 3 more

@alanwest alanwest changed the title Fix issue where when an application has an ExceptionFilter, the excep… Fix ASP.NET Core ExceptionFilter prevents recording exception Jul 26, 2022
Copy link
Member

@alanwest alanwest 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 @al-mac!

If you can make it, we have a .NET SIG meeting tomorrow at 11 am Pacific. I plan to discuss your PR. Part of me wonders if we should do something different in the future to prevent OnException and other methods from being invoked in scenarios we did not originally intend for.

My thought is maybe being explicit with the event names we handle versus doing an EndsWith("Exception") here

else if (value.Key.EndsWith("Exception", StringComparison.Ordinal))
{
this.handler.OnException(Activity.Current, value.Value);
}

@cijothomas
Copy link
Member

Thanks for this PR @al-mac !

@alanwest Please do add this to SIG discussion. I was chatting offline with @vishweshbankwar for some potential improvements in this area as well. (Its related to how leveraging Activity.Current in non Start/End path could result in fetching unintended Activity.)
Given we have reference to AspNetCore.App (for new runtimes..), we should discuss if we can use the Public types which are the payload, instead of relying on reflection.

@al-mac
Copy link
Contributor Author

al-mac commented Jul 26, 2022

@alanwest thanks! Unfortunately I won't be able to attend the meeting. Nice catch on the potential change on the DiagnosticSourceListener.cs. The specific scenario that we worked on this PR will probably never happen if this changes to something like else if (string.Compare(value.Key, "OnException", StringComparison.OrdinalIgnoreCase) == 0). The only concern is if it could break some other use-case, but at this point is valid to consider adding some more else ifs to the code in order to cover those.

@@ -550,6 +552,48 @@ void ConfigureTestServices(IServiceCollection services)
Assert.Equal(shouldEnrichBeCalled, enrichCalled);
}

[Fact]
public async Task ShouldExportActivityWithOneExceptionFilter()
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Both tests could be combined into one like

[Theory]
[InlineData(1)]
[InlineData(2)]
public async Task ShouldExportSingleActivityWithOneOrMoreExceptionFilters(int mode)
.
.
.

@vishweshbankwar
Copy link
Member

Related issue: #2853

@alanwest
Copy link
Member

Related issue: #2853

👍 ah, good find! I've updated the description since this PR should resolve the issue.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Aug 4, 2022
@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Aug 5, 2022
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Aug 13, 2022
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Aug 20, 2022
@utpilla utpilla reopened this Aug 23, 2022
@alanwest
Copy link
Member

@al-mac apologies for the delay on this PR! We just discussed it today and would like to get it merged. Would you mind resolving the conflicts?

@al-mac
Copy link
Contributor Author

al-mac commented Aug 23, 2022

@alanwest not at all! i'll work on it later today

@alanwest alanwest removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Aug 24, 2022
@cijothomas cijothomas merged commit 4310e08 into open-telemetry:main Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExceptionFilter in ASP.NET Core prevents setting error status
6 participants