-
Notifications
You must be signed in to change notification settings - Fork 780
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
Add Support for HttpSemanticConvention breaking changes. #4504
Conversation
@vishweshbankwar please review |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4504 +/- ##
==========================================
+ Coverage 85.16% 85.17% +0.01%
==========================================
Files 316 317 +1
Lines 12583 12594 +11
==========================================
+ Hits 10716 10727 +11
Misses 1867 1867
|
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.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
test/OpenTelemetry.Api.Tests/Internal/HttpSemanticConventionHelperTest.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Api.Tests/Internal/HttpSemanticConventionHelperTest.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, just one more minor thing
namespace OpenTelemetry.Internal | ||
{ |
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.
just one more nit. use a file-scoped namespace here to reduce indentation
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.
no problem!
any way to make this a code style rule for future PRs?
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 should also update the existing classes to use file-scoped namespace for being consistent.
Design discussion issue #4484
Http Semantic Convention is introducing breaking changes.
This PR introduces a new helper class to read and parse the new environment variable.
During startup, an instrumentation library should check for an environment variable.
Based on the parsed value, that library will either emit the old attributes, new attributes, or both.
Changes
HttpInListener
ctor inInstrumentation.AspNetCore
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes