Enable OnException for AspNetCore instrumentation

[eddynaka]

Fixes #1405.

Changes

Please provide a brief description of the changes here.

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

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

[codecov]

Codecov Report

Merging #1408 into master will decrease coverage by 0.06%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1408      +/-   ##
==========================================
- Coverage   81.33%   81.26%   -0.07%     
==========================================
  Files         227      227              
  Lines        6097     6112      +15     
==========================================
+ Hits         4959     4967       +8     
- Misses       1138     1145       +7     

Impacted Files	Coverage Δ
...tation.AspNetCore/Implementation/HttpInListener.cs	84.29% <64.28%> (-2.62%)	⬇️
...ion.AspNetCore/AspNetCoreInstrumentationOptions.cs	100.00% <100.00%> (ø)
...us/Implementation/PrometheusExporterEventSource.cs	63.63% <0.00%> (-9.10%)	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	79.41% <0.00%> (-2.95%)	⬇️

[reyang]

@pakrym FYI.

[pakrym]

@pakrym FYI.

Does this mean that Azure SDKs should put the message into description as well?

[cijothomas]

@pakrym FYI.

Does this mean that Azure SDKs should put the message into description as well?

Not sure I understood. Could your clarify?
This PR is responding to one more DS event from AspNetCore. This event gives Exception, which is stored as ActivityEvent following the semantic conventions. There shouldn't be anything specific to Azure SDK here..

[pakrym]

Recently @reyang mentioned in the email "We SHOULD NOT add any exception object to the Span. How to report an exception is not in the scope this year. I think most likely we will be using logs" but it looks like this PR uses Status.Description to report the exception message. So I'm trying to understand what should we do when reporting exceptions from Azure SDKs.

[cijothomas]

Recently @reyang mentioned in the email "We SHOULD NOT add any exception object to the Span. How to report an exception is not in the scope this year. I think most likely we will be using logs" but it looks like this PR uses Status.Description to report the exception message. So I'm trying to understand what should we do when reporting exceptions from Azure SDKs.

How to report an exception is not in the scope this year

This is probably specific to Microsoft specific Exporters. We can discuss it offline.

This PR is just making this repo compliant to the OpenTelemetry specs:
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#status

[CodeBlanch]

@eddynaka Are you going to add a test for this?

[eddynaka]

@eddynaka Are you going to add a test for this?

i was thinking how i would do that.

[CodeBlanch]

@eddynaka For the test...

opentelemetry-dotnet/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs

Lines 146 to 152 in 41cd4f5

	public override async Task<bool> ProcessAsync(HttpContext context)
	{
	context.Response.StatusCode = this.statusCode;
	context.Response.HttpContext.Features.Get<IHttpResponseFeature>().ReasonPhrase = this.reasonPhrase;
	await context.Response.WriteAsync("empty");
	return false;
	}

I think all you need is for that callback to throw an unhandled exception. You could have the test add something to request query string like "?throw=true" which you could detect off the context in the callback? Might work 😄

[eddynaka]

@eddynaka For the test...

opentelemetry-dotnet/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs

Lines 146 to 152 in 41cd4f5

	public override async Task<bool> ProcessAsync(HttpContext context)
	{
	context.Response.StatusCode = this.statusCode;
	context.Response.HttpContext.Features.Get<IHttpResponseFeature>().ReasonPhrase = this.reasonPhrase;
	await context.Response.WriteAsync("empty");
	return false;
	}

I think all you need is for that callback to throw an unhandled exception. You could have the test add something to request query string like "?throw=true" which you could detect off the context in the callback? Might work 😄

Thanks for the guidance! just pushed one change testing the exception.
Let me know what do you think!