-
Notifications
You must be signed in to change notification settings - Fork 765
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
update HttpSemanticConventions for Instrumentation.Http #4538
Conversation
FYI - we will also need to update this https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs (separate PR) |
Sorry, I missed it because I was only looking at the "Listener" classes. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4538 +/- ##
==========================================
- Coverage 85.04% 84.91% -0.13%
==========================================
Files 313 313
Lines 12575 12601 +26
==========================================
+ Hits 10694 10700 +6
- Misses 1881 1901 +20
|
src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.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.
LGTM: Need changelog entry
src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
|
||
if (request.Headers.TryGetValues("User-Agent", out var userAgentValues)) | ||
{ | ||
var userAgent = userAgentValues.FirstOrDefault(); |
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.
This would cause an allocation. Refer to how AspNetCore instrumentation handles this: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L217-L224
This should be done in a separate PR which updates the benchmarks as well.
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.
Left a comment about updating the urls to point to a commit/tag instead of main branch.
Need a CHANGELOG entry.
@@ -110,5 +110,15 @@ internal static class SemanticConventions | |||
public const string AttributeExceptionType = "exception.type"; | |||
public const string AttributeExceptionMessage = "exception.message"; | |||
public const string AttributeExceptionStacktrace = "exception.stacktrace"; | |||
|
|||
// NEW v1.21.0 Http Semantic Conventions |
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.
What does this mean?
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.
I was asked to separate the new attributes from the others and give it a descriptive comment.
The word "new" isn't needed here, sorry for the confusion.
I've updated this comment in the AspNetCore PR but forgot to update it 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.
I was asked to separate the new attributes from the others and give it a descriptive comment.
What does v1.21.0 mean?
The word "new" isn't needed here
👍
@@ -110,5 +110,15 @@ internal static class SemanticConventions | |||
public const string AttributeExceptionType = "exception.type"; | |||
public const string AttributeExceptionMessage = "exception.message"; | |||
public const string AttributeExceptionStacktrace = "exception.stacktrace"; | |||
|
|||
// Http v1.21.0 https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md |
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.
FYI - v1.21.0 doesn't contain semantic convention changes https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/CHANGELOG.md#v1210-2023-05-09
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.
The semantic-conventions repo doesn't have any tags yet so I can't link to a versioned copy of the Http document.
I discussed this with Utkarsh last week and he suggested using the v1.21.0 tag on the opentelemetry-specification repo.
The opentelemetry-specification repo does have tags and that version of the document does include all of the new attributes that we're adding in this PR.
Design discussion issue #4484
Changes
SemanticConventions
Instrumentation.Http
classesHttpHandlerDiagnosticListener
andHttpHandlerMetricsDiagnosticListener
to emit new attributesPlease provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes@vishweshbankwar please review