Skip to content

Commit 31ee947

Browse files
utpillautpilla
authored
Merge branch 'main' into utpilla/Update-Histogram-Buckets
2 parents 9b70052 + d91be77 commit 31ee947

File tree

7 files changed

+120
-25
lines changed

7 files changed

+120
-25
lines changed

‎src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreMetrics.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ internal sealed class AspNetCoreMetrics : IDisposable
3636
"Microsoft.AspNetCore.Hosting.HttpRequestIn",
3737
"Microsoft.AspNetCore.Hosting.HttpRequestIn.Start",
3838
"Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop",
39+
"Microsoft.AspNetCore.Diagnostics.UnhandledException",
40+
"Microsoft.AspNetCore.Hosting.UnhandledException",
3941
};
4042

4143
private readonly Func<string, object, object, bool> isEnabled = (eventName, _, _)

‎src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,14 @@
1818
`http` or `http/dup`.
1919
([#5001](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5001))
2020

21+
* An additional attribute `error.type` will be added to activity and
22+
`http.server.request.duration` metric when the request results in unhandled
23+
exception. The attribute value will be set to full name of exception type.
24+
25+
The attribute will only be added when `OTEL_SEMCONV_STABILITY_OPT_IN`
26+
environment variable is set to `http` or `http/dup`.
27+
([#4986](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4986))
28+
2129
## 1.6.0-beta.2
2230

2331
Released 2023-Oct-26

‎src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -425,6 +425,11 @@ public void OnException(Activity activity, object payload)
425425
return;
426426
}
427427

428+
if (this.emitNewAttributes)
429+
{
430+
activity.SetTag(SemanticConventions.AttributeErrorType, exc.GetType().FullName);
431+
}
432+
428433
if (this.options.RecordException)
429434
{
430435
activity.RecordException(exc);

‎src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs‎

Lines changed: 61 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
using OpenTelemetry.Internal;
2121

2222
#if NET6_0_OR_GREATER
23+
using System.Diagnostics.CodeAnalysis;
2324
using Microsoft.AspNetCore.Routing;
2425
#endif
2526
using OpenTelemetry.Trace;
@@ -32,9 +33,14 @@ internal sealed class HttpInMetricsListener : ListenerHandler
3233
internal const string HttpServerDurationMetricName = "http.server.duration";
3334
internal const string HttpServerRequestDurationMetricName = "http.server.request.duration";
3435

36+
internal const string OnUnhandledHostingExceptionEvent = "Microsoft.AspNetCore.Hosting.UnhandledException";
37+
internal const string OnUnhandledDiagnosticsExceptionEvent = "Microsoft.AspNetCore.Diagnostics.UnhandledException";
3538
private const string OnStopEvent = "Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop";
3639
private const string EventName = "OnStopActivity";
3740
private const string NetworkProtocolName = "http";
41+
private static readonly PropertyFetcher<Exception> ExceptionPropertyFetcher = new("Exception");
42+
private static readonly PropertyFetcher<HttpContext> HttpContextPropertyFetcher = new("HttpContext");
43+
private static readonly object ErrorTypeHttpContextItemsKey = new();
3844

3945
private readonly Meter meter;
4046
private readonly AspNetCoreMetricsInstrumentationOptions options;
@@ -66,23 +72,65 @@ internal HttpInMetricsListener(string name, Meter meter, AspNetCoreMetricsInstru
6672

6773
public override void OnEventWritten(string name, object payload)
6874
{
69-
if (name == OnStopEvent)
75+
switch (name)
7076
{
71-
if (this.emitOldAttributes)
72-
{
73-
this.OnEventWritten_Old(name, payload);
74-
}
77+
case OnUnhandledDiagnosticsExceptionEvent:
78+
case OnUnhandledHostingExceptionEvent:
79+
{
80+
if (this.emitNewAttributes)
81+
{
82+
this.OnExceptionEventWritten(name, payload);
83+
}
84+
}
85+
86+
break;
87+
case OnStopEvent:
88+
{
89+
if (this.emitOldAttributes)
90+
{
91+
this.OnEventWritten_Old(name, payload);
92+
}
93+
94+
if (this.emitNewAttributes)
95+
{
96+
this.OnEventWritten_New(name, payload);
97+
}
98+
}
99+
100+
break;
101+
}
102+
}
75103

76-
if (this.emitNewAttributes)
77-
{
78-
this.OnEventWritten_New(name, payload);
79-
}
104+
public void OnExceptionEventWritten(string name, object payload)
105+
{
106+
// We need to use reflection here as the payload type is not a defined public type.
107+
if (!TryFetchException(payload, out Exception exc) || !TryFetchHttpContext(payload, out HttpContext ctx))
108+
{
109+
AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), nameof(this.OnExceptionEventWritten), HttpServerDurationMetricName);
110+
return;
80111
}
112+
113+
ctx.Items.Add(ErrorTypeHttpContextItemsKey, exc.GetType().FullName);
114+
115+
// See https://github.com/dotnet/aspnetcore/blob/690d78279e940d267669f825aa6627b0d731f64c/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L252
116+
// and https://github.com/dotnet/aspnetcore/blob/690d78279e940d267669f825aa6627b0d731f64c/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs#L174
117+
// this makes sure that top-level properties on the payload object are always preserved.
118+
#if NET6_0_OR_GREATER
119+
[UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "The ASP.NET Core framework guarantees that top level properties are preserved")]
120+
#endif
121+
static bool TryFetchException(object payload, out Exception exc)
122+
=> ExceptionPropertyFetcher.TryFetch(payload, out exc) && exc != null;
123+
#if NET6_0_OR_GREATER
124+
[UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "The ASP.NET Core framework guarantees that top level properties are preserved")]
125+
#endif
126+
static bool TryFetchHttpContext(object payload, out HttpContext ctx)
127+
=> HttpContextPropertyFetcher.TryFetch(payload, out ctx) && ctx != null;
81128
}
82129

83130
public void OnEventWritten_Old(string name, object payload)
84131
{
85132
var context = payload as HttpContext;
133+
86134
if (context == null)
87135
{
88136
AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), EventName, HttpServerDurationMetricName);
@@ -170,6 +218,10 @@ public void OnEventWritten_New(string name, object payload)
170218
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRoute, route));
171219
}
172220
#endif
221+
if (context.Items.TryGetValue(ErrorTypeHttpContextItemsKey, out var errorType))
222+
{
223+
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeErrorType, errorType));
224+
}
173225

174226
// We are relying here on ASP.NET Core to set duration before writing the stop event.
175227
// https://github.com/dotnet/aspnetcore/blob/d6fa351048617ae1c8b47493ba1abbe94c3a24cf/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L449

‎src/Shared/SemanticConventions.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ internal static class SemanticConventions
110110
public const string AttributeExceptionType = "exception.type";
111111
public const string AttributeExceptionMessage = "exception.message";
112112
public const string AttributeExceptionStacktrace = "exception.stacktrace";
113+
public const string AttributeErrorType = "error.type";
113114

114115
// v1.21.0
115116
// https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md

‎test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,8 @@ public IncomingRequestsCollectionsIsAccordingToTheSpecTests_New(WebApplicationFa
3939
}
4040

4141
[Theory]
42-
[InlineData("/api/values", null, "user-agent", 503, "503")]
43-
[InlineData("/api/values", "?query=1", null, 503, null)]
42+
[InlineData("/api/values", null, "user-agent", 200, null)]
43+
[InlineData("/api/values", "?query=1", null, 200, null)]
4444
[InlineData("/api/exception", null, null, 503, null)]
4545
[InlineData("/api/exception", null, null, 503, null, true)]
4646
public async Task SuccessfulTemplateControllerCallGeneratesASpan_New(
@@ -123,6 +123,7 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan_New(
123123
if (statusCode == 503)
124124
{
125125
Assert.Equal(ActivityStatusCode.Error, activity.Status);
126+
Assert.Equal("System.Exception", activity.GetTagValue(SemanticConventions.AttributeErrorType));
126127
}
127128
else
128129
{

‎test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs‎

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -185,8 +185,10 @@ public async Task ValidateNet8RateLimitingMetricsAsync()
185185
}
186186
#endif
187187

188-
[Fact]
189-
public async Task RequestMetricIsCaptured_New()
188+
[Theory]
189+
[InlineData("/api/values/2", "api/Values/{id}", null, 200)]
190+
[InlineData("/api/Error", "api/Error", "System.Exception", 500)]
191+
public async Task RequestMetricIsCaptured_New(string api, string expectedRoute, string expectedErrorType, int expectedStatusCode)
190192
{
191193
var configuration = new ConfigurationBuilder()
192194
.AddInMemoryCollection(new Dictionary<string, string> { [SemanticConventionOptInKeyName] = "http" })
@@ -207,11 +209,15 @@ public async Task RequestMetricIsCaptured_New()
207209
})
208210
.CreateClient())
209211
{
210-
using var response1 = await client.GetAsync("/api/values").ConfigureAwait(false);
211-
using var response2 = await client.GetAsync("/api/values/2").ConfigureAwait(false);
212-
213-
response1.EnsureSuccessStatusCode();
214-
response2.EnsureSuccessStatusCode();
212+
try
213+
{
214+
using var response = await client.GetAsync(api).ConfigureAwait(false);
215+
response.EnsureSuccessStatusCode();
216+
}
217+
catch
218+
{
219+
// ignore error.
220+
}
215221
}
216222

217223
// We need to let End callback execute as it is executed AFTER response was returned.
@@ -229,12 +235,14 @@ public async Task RequestMetricIsCaptured_New()
229235

230236
Assert.Equal("s", metric.Unit);
231237
var metricPoints = GetMetricPoints(metric);
232-
Assert.Equal(2, metricPoints.Count);
238+
Assert.Single(metricPoints);
233239

234240
AssertMetricPoints_New(
235241
metricPoints: metricPoints,
236-
expectedRoutes: new List<string> { "api/Values", "api/Values/{id}" },
237-
expectedTagsCount: 6);
242+
expectedRoutes: new List<string> { expectedRoute },
243+
expectedErrorType,
244+
expectedStatusCode,
245+
expectedTagsCount: expectedErrorType == null ? 6 : 7);
238246
}
239247

240248
[Theory]
@@ -430,6 +438,8 @@ public async Task RequestMetricIsCaptured_Dup()
430438
AssertMetricPoints_New(
431439
metricPoints: metricPoints,
432440
expectedRoutes: new List<string> { "api/Values", "api/Values/{id}" },
441+
null,
442+
200,
433443
expectedTagsCount: 6);
434444
}
435445
#endif
@@ -456,6 +466,8 @@ private static List<MetricPoint> GetMetricPoints(Metric metric)
456466
private static void AssertMetricPoints_New(
457467
List<MetricPoint> metricPoints,
458468
List<string> expectedRoutes,
469+
string expectedErrorType,
470+
int expectedStatusCode,
459471
int expectedTagsCount)
460472
{
461473
// Assert that one MetricPoint exists for each ExpectedRoute
@@ -476,7 +488,7 @@ private static void AssertMetricPoints_New(
476488

477489
if (metricPoint.HasValue)
478490
{
479-
AssertMetricPoint_New(metricPoint.Value, expectedRoute, expectedTagsCount);
491+
AssertMetricPoint_New(metricPoint.Value, expectedStatusCode, expectedRoute, expectedErrorType, expectedTagsCount);
480492
}
481493
else
482494
{
@@ -519,8 +531,10 @@ private static void AssertMetricPoints_Old(
519531

520532
private static KeyValuePair<string, object>[] AssertMetricPoint_New(
521533
MetricPoint metricPoint,
522-
string expectedRoute = "api/Values",
523-
int expectedTagsCount = StandardTagsCount)
534+
int expectedStatusCode,
535+
string expectedRoute,
536+
string expectedErrorType,
537+
int expectedTagsCount)
524538
{
525539
var count = metricPoint.GetHistogramCount();
526540
var sum = metricPoint.GetHistogramSum();
@@ -540,7 +554,7 @@ private static KeyValuePair<string, object>[] AssertMetricPoint_New(
540554

541555
var method = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRequestMethod, "GET");
542556
var scheme = new KeyValuePair<string, object>(SemanticConventions.AttributeUrlScheme, "http");
543-
var statusCode = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpResponseStatusCode, 200);
557+
var statusCode = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpResponseStatusCode, expectedStatusCode);
544558
var flavor = new KeyValuePair<string, object>(SemanticConventions.AttributeNetworkProtocolVersion, "1.1");
545559
var route = new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRoute, expectedRoute);
546560
Assert.Contains(method, attributes);
@@ -549,6 +563,18 @@ private static KeyValuePair<string, object>[] AssertMetricPoint_New(
549563
Assert.Contains(flavor, attributes);
550564
Assert.Contains(route, attributes);
551565

566+
if (expectedErrorType != null)
567+
{
568+
#if NET8_0_OR_GREATER
569+
// Expected to change in next release
570+
// https://github.com/dotnet/aspnetcore/issues/51029
571+
var errorType = new KeyValuePair<string, object>("exception.type", expectedErrorType);
572+
#else
573+
var errorType = new KeyValuePair<string, object>(SemanticConventions.AttributeErrorType, expectedErrorType);
574+
#endif
575+
Assert.Contains(errorType, attributes);
576+
}
577+
552578
// Inspect Histogram Bounds
553579
var histogramBuckets = metricPoint.GetHistogramBuckets();
554580
var histogramBounds = new List<double>();

0 commit comments

Comments
 (0)