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

Http server semconv span stable #4978

Open
wants to merge 46 commits into
base: main
Choose a base branch
from

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Sep 4, 2024

Updates semconv to 1.27 for server spans.

Metrics to be done in followup.

dyladan and others added 28 commits August 21, 2024 16:48
…tils.ts

Co-authored-by: Marc Pichler <marcpi@edu.aau.at>
)

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Co-authored-by: Mend Renovate <bot@renovateapp.com>
@dyladan dyladan force-pushed the http-server-semconv-span-stable branch from d2b6fda to 9a0c2de Compare September 4, 2024 19:55
@@ -80,6 +80,7 @@
"@opentelemetry/core": "1.26.0",
"@opentelemetry/instrumentation": "0.53.0",
"@opentelemetry/semantic-conventions": "1.27.0",
"forwarded-parse": "2.1.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

Note

NEW DEPENDENCY

This is needed to parse the somewhat complex forwarded header

Copy link
Member

Choose a reason for hiding this comment

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

thanks for pointing this out - I also had a quick look and it does not seem like there's a easy way to do it 😞
I think using the library is justified.

@dyladan dyladan marked this pull request as ready for review September 11, 2024 12:57
@dyladan dyladan requested a review from a team September 11, 2024 12:57
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.39%. Comparing base (f8ab559) to head (1054a44).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4978   +/-   ##
=======================================
  Coverage   93.39%   93.39%           
=======================================
  Files          46       46           
  Lines         712      712           
  Branches      120      120           
=======================================
  Hits          665      665           
  Misses         47       47           

JamieDanielson
JamieDanielson previously approved these changes Sep 16, 2024
Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

@@ -76,7 +76,7 @@ The following options are deprecated:

## Semantic Conventions

### Client Spans
### Client and Server Spans
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how best to update without making this big PR even bigger, but the section with the table of attributes will need to be updated in some way (starting on line 114 that still references Server Spans). It may be worth having two tables - 1 representing old, 1 representing new. I'm happy to help craft that table in a separate issue and we can issue a follow-up PR. I would recommend at least a small change to that section though along the lines of "These are the attributes in the default behavior using only experimental attributes".

Copy link
Member Author

Choose a reason for hiding this comment

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

I left the old table as-is for now. The new semconv is documented already in the semconv repo. When we remove this fallback the old table will be removed entirely. Would that be sufficient or do you think we need a table here as well?

Copy link
Member

Choose a reason for hiding this comment

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

I've gotten a lot of requests for showing what attributes are included in instrumentation, especially as different instrumentations (and languages) are in different versions. Folks also ask about other attributes (non-semconv) in different instrumentations, e.g. express.name, though that's less relevant for http here. The semconv might be in the semconv repo but it doesn't specify what we include, and may not be easily navigable by end users. Ideally we autogenerate a list of attributes vs handcrafting a table though, but with the table already there we may as well use it.

experimental/CHANGELOG.md Outdated Show resolved Hide resolved
experimental/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -80,6 +80,7 @@
"@opentelemetry/core": "1.26.0",
"@opentelemetry/instrumentation": "0.53.0",
"@opentelemetry/semantic-conventions": "1.27.0",
"forwarded-parse": "2.1.2",
Copy link
Member

Choose a reason for hiding this comment

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

thanks for pointing this out - I also had a quick look and it does not seem like there's a easy way to do it 😞
I think using the library is justified.

@@ -46,9 +49,12 @@ import {
SEMATTRS_HTTP_METHOD,
SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH_UNCOMPRESSED,
SEMATTRS_HTTP_ROUTE,
SEMATTRS_HTTP_SCHEME,
Copy link
Member

Choose a reason for hiding this comment

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

Actually, hm. I wonder if we should use the new ATTR_ imports here from incubating. So the ATTR_ stable attributes should still get imported as-is, but the ATTR_ incubating/experimental attributes should be used and imported from semantic-conventions/incubating.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, this is probably right. It feels a bit awkward to have a mix of SEMATTRS_* and ATTRS_* but one of the reasons to avoid using the new attributes is if anything has changed. For example, we've been advised not to update db attributes because they are close to stability and we may end up with multiple breaking changes before then. Maybe we only use the new ATTRS_* if something didn't exist in the previous version

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I decided to leave the old attribute names to be sure there isn't a change/regression

@JamieDanielson JamieDanielson dismissed their stale review September 18, 2024 21:24

I think we may want to use the new ATTR_ imports even for old semconv. So the ATTR_ stable attributes should still get imported as-is, but the ATTR_ incubating/experimental attributes should be used and imported from semantic-conventions/incubating.

@dyladan dyladan requested a review from a team as a code owner September 20, 2024 12:29
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.

5 participants