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

feat: improve http span name #3603

Merged
merged 6 commits into from
Feb 28, 2023
Merged

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Feb 10, 2023

Short description of the changes

Remove HTTP prefix in span name to match HTTP semantic conventions 1.18.
Set server span name with route information if present.

Refs: open-telemetry/opentelemetry-specification#3165

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Unittests

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

Remove HTTP prefix in span name to match latest semantic conventions.

Set server span name with route information if present.
@Flarna Flarna requested a review from a team February 10, 2023 23:18
@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #3603 (4c06727) into main (abfe059) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 4c06727 differs from pull request most recent head ac99ffc. Consider uploading reports for the commit ac99ffc to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3603      +/-   ##
==========================================
- Coverage   93.63%   93.62%   -0.02%     
==========================================
  Files         265      265              
  Lines        7417     7417              
  Branches     1511     1511              
==========================================
- Hits         6945     6944       -1     
- Misses        472      473       +1     
Impacted Files Coverage Δ
...-trace-base/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

experimental/CHANGELOG.md Outdated Show resolved Hide resolved
@MSNev
Copy link
Contributor

MSNev commented Feb 11, 2023

Technically, not a "breaking" change. However, it should be considered a behavioral change as any downstream users who are currently expecting client requests to contain the HTTP prefix might raise a bug against this.

It's probably worth calling this out in the release notes a little more.

@@ -8,6 +8,7 @@ All notable changes to experimental packages in this project will be documented

### :rocket: (Enhancement)

* feat: improve http span name [#3603](https://github.com/open-telemetry/opentelemetry-js/pull/3603) @Flarna
Copy link
Member

Choose a reason for hiding this comment

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

I think this is considered a breaking change since any queries by span name will break

Copy link
Member Author

Choose a reason for hiding this comment

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

It was not marked as breaking in spec so why here?

Copy link
Member Author

Choose a reason for hiding this comment

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

anyhow, added an entry in breaking changes and adapted wording in feat section

Comment on lines +716 to +719
const route = attributes[SemanticAttributes.HTTP_ROUTE];
if (route) {
span.updateName(`${request.method || 'GET'} ${route}`);
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe an entry for this new feature in the changelog as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@Flarna Flarna added enhancement New feature or request instrumentation spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug labels Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants