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

[Instrumentation.AspNet] Spans - semantic convention v1.24.0 #1607

Merged
merged 25 commits into from
Mar 25, 2024

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Mar 11, 2024

Fixes N/A

Changes

Span attributes based on HTTP semantic convention v1.24.0 - Common and Server part sections.

Span names:

Common part

  • error.type - ccb1d4f
  • http.request.method and http.request.method_original - e31b687
  • http.response.header.<key> - Opn-In - skipping for now
  • http.response.status_code - 5677f4f
  • network.peer.address and network.peer.port - Recommended - skipping for now
  • network.protocol.name - empty, means HTTP by default
  • network.protocol.version - a794951
  • network.transport - Opn-In - skipping for now

Server part:

  • client.address - Recommended - skipping for now
  • client.port - Opt-In skipping for now
  • http.request.header.<key> - Opt-In skipping for now
  • http.route - already handled, no changes
  • network.local.address and network.local.port - Opt-In skipping for now
  • server.address and server.port - 3d3ce2e
  • url.path - 86fdf5a
  • url.query - e920468
  • url.scheme - 445a684
  • user_agent.original - 78e266a

Consider to review commits separately.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • [ ] Design discussion issue #
  • [ ] Changes in public API reviewed

@Kielek Kielek added the comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet label Mar 11, 2024
@Kielek Kielek changed the title [Instrumentation.AspNet] Spans - semantic convention v.1.24.0 [Instrumentation.AspNet] Spans - semantic convention v1.24.0 Mar 11, 2024
Copy link

codecov bot commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 97.56098% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 65.84%. Comparing base (71655ce) to head (8d441c0).
Report is 167 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1607      +/-   ##
==========================================
- Coverage   73.91%   65.84%   -8.08%     
==========================================
  Files         267      233      -34     
  Lines        9615     8728     -887     
==========================================
- Hits         7107     5747    -1360     
- Misses       2508     2981     +473     
Flag Coverage Δ
unittests-Extensions 82.60% <ø> (?)
unittests-ResourceDetectors.ProcessRuntime 93.33% <ø> (?)
unittests-Solution 76.26% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ion.AspNet/Implementation/HttpInMetricsListener.cs 97.36% <100.00%> (-2.64%) ⬇️
...ntation.AspNet/Implementation/RequestDataHelper.cs 100.00% <100.00%> (ø)
...umentation.AspNet/Implementation/HttpInListener.cs 87.50% <95.83%> (-0.82%) ⬇️

... and 220 files with indirect coverage changes

@Kielek Kielek marked this pull request as ready for review March 12, 2024 06:01
@Kielek Kielek requested a review from a team March 12, 2024 06:01
{
activity.SetTag(SemanticConventions.AttributeHttpHost, request.Url.Host + ":" + request.Url.Port);
if (query.StartsWith("?", StringComparison.InvariantCulture))
Copy link
Member

Choose a reason for hiding this comment

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

@Kielek - Does spec requires it to be without ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if it hard requierment, but all examples are without this sign.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it doesn't include the ? component, https://www.rfc-editor.org/rfc/rfc3986#section-3.4

@@ -8,6 +8,9 @@
* **Breaking Change**: `server.address` and `server.port` no longer added
for `http.server.request.duration` metric.
([#1606](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1606))
* **Breaking change** Spans names and attributes
Copy link
Member

Choose a reason for hiding this comment

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

Could you please include the details on new versus old attributes?

Something like this: https://opentelemetry.io/blog/2023/http-conventions-declared-stable/#summary-of-changes

Copy link
Member

@vishweshbankwar vishweshbankwar left a comment

Choose a reason for hiding this comment

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

LGTM overall - suggested change on changelog.

@Kielek
Copy link
Contributor Author

Kielek commented Mar 25, 2024

@vishweshbankwar, changes in changelog: fb25884, please review once more time.

Co-authored-by: Vishwesh Bankwar <vishweshbankwar@users.noreply.github.com>
@Kielek Kielek merged commit ac4fe15 into open-telemetry:main Mar 25, 2024
112 of 113 checks passed
@Kielek Kielek deleted the aspnet-traces branch March 25, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants