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.

Sign up for GitHub

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

Jump to bottom

feat(opentelemetry-js): add content size attributes to HTTP spans #1625

Merged
merged 15 commits into from
Dec 10, 2020
Merged

feat(opentelemetry-js): add content size attributes to HTTP spans #1625

merged 15 commits into from
Dec 10, 2020

Conversation

nijotz
Copy link
Contributor

@nijotz nijotz commented Oct 26, 2020 •
edited
Loading

Which problem is this PR solving?

Short description of the changes

  • Spec change is here
  • Uses HTTP headers to determine if content is compressed.
  • Uses HTTP headers to determine content size
  • Defines the 4 new attributes in the spec change

Carlo Pearson and others added 3 commits October 26, 2020 13:56
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 26, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

@nijotz nijotz changed the title Http content size feat(opentelemetry-js): add content size attributes to HTTP spans Oct 26, 2020
@nijotz nijotz mentioned this pull request Oct 26, 2020
Closed
@codecov
Copy link

codecov bot commented Oct 26, 2020 •
edited
Loading

Codecov Report

Merging #1625 (2698c39) into master (b260f89) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1625      +/-   ##
==========================================
+ Coverage   91.94%   91.98%   +0.03%     
==========================================
  Files         167      167              
  Lines        5573     5599      +26     
  Branches     1186     1193       +7     
==========================================
+ Hits         5124     5150      +26     
  Misses        449      449
Impacted Files Coverage Δ
packages/opentelemetry-plugin-http/src/utils.ts 98.05% <100.00%> (+0.28%) ⬆️

@dyladan dyladan linked an issue Oct 28, 2020 that may be closed by this pull request
Add HTTP semantic convention for content size #1249
Closed
@obecny obecny added the enhancement New feature or request label Oct 30, 2020
@dyladan
Copy link
Member

dyladan commented Nov 4, 2020

Still waiting on this update #1625 (comment)

Also, there are now conflicts.

@carlo-808
Copy link
Contributor

Still waiting on this update #1625 (comment)

Also, there are now conflicts.

Hey @dyladan , we pushed up some changes last Thursday and resolved the conflicts. Please take a look at our changes.

@dyladan
Copy link
Member

dyladan commented Nov 18, 2020

@open-telemetry/javascript-approvers lets get this reviewed as it is one of the last things we have for GA

@carlo-808
Copy link
Contributor

carlo-808 commented Nov 18, 2020 •
edited
Loading

@dyladan and @obecny , I realized we need to make one more small change with this story. We need to make sure we only grab the last item of the content-encoding header. I'll make that change today.
Based on @nijotz comment below, we believe this is ready for review.

@nijotz
Copy link
Contributor Author

nijotz commented Nov 18, 2020

So, there was some concern that if the content encoding header was gzip, identity that would mean the content was uncompressed and the if statement would fail. But after some discussion and research and testing, we're confident that this code will work as-is. The only way that the Content-Encoding header can have a value and not be compressed is if the value is identity

@dyladan
Copy link
Member

dyladan commented Nov 25, 2020

Any update?

@nijotz
Copy link
Contributor Author

nijotz commented Nov 25, 2020

Still good-to-go, waiting on review

@vmarchaud vmarchaud mentioned this pull request Dec 5, 2020
Closed
@dyladan
Copy link
Member

dyladan commented Dec 9, 2020

@open-telemetry/javascript-approvers can I get one more ✔️ here?

@nijotz
Copy link
Contributor Author

nijotz commented Dec 9, 2020

Good catches @obecny, working on an update now

@nijotz
Copy link
Contributor Author

nijotz commented Dec 9, 2020

Just pushed a commit to make changes requested by @obecny

obecny
obecny approved these changes Dec 10, 2020
View reviewed changes
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, please don't force push after 1st review as it is impossible to see changes then, thx

@obecny
Copy link
Member

obecny commented Dec 10, 2020

retriggering EasyCLA

@obecny obecny closed this Dec 10, 2020
@obecny
Copy link
Member

obecny commented Dec 10, 2020

retriggering EasyCLA

@obecny obecny reopened this Dec 10, 2020
@obecny
Copy link
Member

obecny commented Dec 10, 2020

the same thing needs to be create for http instrumentation -> #1740 (in separate PR)

@obecny obecny merged commit 52c6096 into open-telemetry:master Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlo-808 carlo-808 carlo-808 left review comments

@vmarchaud vmarchaud vmarchaud approved these changes

@dyladan dyladan dyladan approved these changes

@obecny obecny obecny approved these changes

@legendecas legendecas Awaiting requested review from legendecas

@markwolff markwolff Awaiting requested review from markwolff

@mayurkale22 mayurkale22 Awaiting requested review from mayurkale22

@mwear mwear Awaiting requested review from mwear

@naseemkullah naseemkullah Awaiting requested review from naseemkullah

@OlivierAlbertini OlivierAlbertini Awaiting requested review from OlivierAlbertini

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add HTTP semantic convention for content size
5 participants
@nijotz @dyladan @carlo-808 @obecny @vmarchaud