Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Http server semconv span stable #4978
Changes from 35 commits
c5f4b41
3d1b95d
d978eb1
963e19a
dc75e57
c71cdb0
eae1d5c
f7efbe8
2d60245
5031276
10e49f1
e4a36b1
9b5ae5b
83b91f0
61f344d
362a1bc
9e2485d
08c5a9a
4920848
93d43ce
e725146
0635448
253c840
2533480
e4ccec8
1bdce0f
1c9abb5
9a0c2de
3703045
397ebc6
98e02f5
7addab4
12d0489
4c19670
1054a44
966eaa5
090f394
46a42e0
826e79b
0df46dd
01b90c5
bc4d955
52c10d4
8a02c47
6354a8f
c9df445
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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".
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.
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?
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.
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.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.
Note
NEW DEPENDENCY
This is needed to parse the somewhat complex forwarded header
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.
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.