-
Notifications
You must be signed in to change notification settings - Fork 762
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
[ASP.NET Core] Add error.type
attribute for tracing and metrics
#4986
[ASP.NET Core] Add error.type
attribute for tracing and metrics
#4986
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4986 +/- ##
==========================================
+ Coverage 83.38% 83.43% +0.05%
==========================================
Files 296 296
Lines 12385 12403 +18
==========================================
+ Hits 10327 10349 +22
+ Misses 2058 2054 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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. |
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the whole looks good. Left some minor comments.
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
ctx.Items.Add(ErrorTypeKey, exc.GetType().FullName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the framework create an activity even if only add AspNetCore metrics instrumentation? If yes, then I think we could add this information to Activity.Current
. Do you know of any obvious pros and cons to adding this information to HttpContext
vs Activity.Current
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the activity will always be there. Perf was the main reason to choose HttpContext.Items
. However, we could use custom property as well on activity. Here are the results from benchmarking
Results when trace is also enabled and activity will have additional tags.
Method | Mean | Error | StdDev | Median | Gen0 | Allocated |
---|---|---|---|---|---|---|
UseActivityTag | 82.36 ns | 1.691 ns | 1.736 ns | 83.06 ns | 0.0085 | 40 B |
UseActivityCustomProperty | 81.98 ns | 1.133 ns | 1.004 ns | 81.54 ns | - | - |
UseContextItems | 81.11 ns | 1.656 ns | 3.420 ns | 79.67 ns | - | - |
Results when only metrics will be enabled and activity will only have one tag
Method | Mean | Error | StdDev | Gen0 | Allocated |
---|---|---|---|---|---|
UseActivityTag | 55.56 ns | 0.572 ns | 0.535 ns | 0.0085 | 40 B |
UseActivityCustomProperty | 80.94 ns | 0.538 ns | 0.477 ns | - | - |
UseContextItems | 80.50 ns | 0.901 ns | 0.843 ns | - | - |
Based on above, I have updated the implementation to use custom property. We could follow the same thing on HttpClient side where we need to add error.type
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC the first time you add a custom property to activity you pay for an allocation. Are we already adding one somewhere else? Seems a bit odd that the allocation isn't showing in the benchmark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CodeBlanch Good catch. The alloc is not showing up in the benchmark as it is getting ignored due to multiple runs. I have switched back to using context.items and it improved the look up as well using the minor tweak on the context key suggested by you. here are the results.
Method | Mean | Error | StdDev | Gen0 | Allocated |
---|---|---|---|---|---|
UseActivityTag | 56.93 ns | 0.572 ns | 0.446 ns | 0.0085 | 40 B |
UseActivityCustomProperty | 83.61 ns | 0.546 ns | 0.511 ns | - | - |
UseContextItemsObjectKey | 68.49 ns | 0.429 ns | 0.401 ns | - | - |
UseContextItemsStringKey | 84.91 ns | 1.191 ns | 0.994 ns | - | - |
test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs
Outdated
Show resolved
Hide resolved
…om/vishweshbankwar/opentelemetry-dotnet into vibankwa/add-error-type-aspnetcore
…om/vishweshbankwar/opentelemetry-dotnet into vibankwa/add-error-type-aspnetcore
@@ -130,6 +171,7 @@ public void OnEventWritten_Old(string name, object payload) | |||
|
|||
public void OnEventWritten_New(string name, object payload) | |||
{ | |||
var activity = Activity.Current; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a Debug.Assert(activity != null)
here. I'm paranoid 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #
Design discussion issue #
Changes
Adds
error.type
attribute to trace and metrics.https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md
https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md
.NET8.0
implementation of metrics ignores settingerror.type
to status code value in case of failure as per the spec. I have opened an issue to get this fixed dotnet/aspnetcore#51620. For now, the issue is targeted to be fixed in.NET9.0
.The change in this PR ensures that the value is consistent with how
.NET8.0
is setting this attribute in metrics in order to avoid breaking change for users migrating from lower versions to.NET8.0
.This change does not resolve
#4865
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes