Skip to content
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

Change http.path to http.target in aspnet and aspnetcore instrumentations #2266

Conversation

vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Aug 19, 2021

http.path is not a semantic convention defined by spec. http.path was used in OpenCensus and is mapped to http.target in OpenTelemetry as per this

@codecov
Copy link

codecov bot commented Aug 19, 2021

Codecov Report

Merging #2266 (ffaa169) into main (d649b3d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2266      +/-   ##
==========================================
+ Coverage   79.09%   79.12%   +0.02%     
==========================================
  Files         238      238              
  Lines        7488     7488              
==========================================
+ Hits         5923     5925       +2     
+ Misses       1565     1563       -2     
Impacted Files Coverage Δ
...umentation.AspNet/Implementation/HttpInListener.cs 88.88% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs 86.23% <100.00%> (ø)
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 97.14% <0.00%> (-2.86%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+5.88%) ⬆️
...Zipkin/Implementation/ZipkinExporterEventSource.cs 72.72% <0.00%> (+9.09%) ⬆️

@vishweshbankwar vishweshbankwar marked this pull request as ready for review August 19, 2021 21:41
@vishweshbankwar vishweshbankwar requested a review from a team August 19, 2021 21:41
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, consider updating the changelog since this will affect the user.

@utpilla
Copy link
Contributor

utpilla commented Aug 19, 2021

We should update SemanticConventions.cs and SpanAttributeConstants.cs in the contrib repo as well to reflect these changes to stay consistent.

https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.Contrib.Shared/Api/SemanticConventions.cs#L52

https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.Contrib.Shared/Api/SpanAttributeConstants.cs#L28

Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Need to update the contrib repo files with the same changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants