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(http-instrumentation): add content size attributes to spans #1771

Merged

Conversation

vmarchaud
Copy link
Member

This is a pure copy/paste of the original commit with the exact same diff.

Fixes #1740

@codecov
Copy link

codecov bot commented Dec 19, 2020

Codecov Report

Merging #1771 (513f7c7) into master (80ea2e0) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1771      +/-   ##
==========================================
+ Coverage   92.15%   92.18%   +0.03%     
==========================================
  Files         165      165              
  Lines        5568     5594      +26     
  Branches     1197     1204       +7     
==========================================
+ Hits         5131     5157      +26     
  Misses        437      437              
Impacted Files Coverage Δ
...es/opentelemetry-instrumentation-http/src/utils.ts 98.06% <100.00%> (+0.27%) ⬆️

@vmarchaud vmarchaud force-pushed the http-instrumentation-content-size branch from bb283aa to 2fff599 Compare December 19, 2020 13:23
* @param { IncomingMessage } Request object whose headers will be analyzed
* @param { Attributes } Attributes object to be modified
*/
export const setRequestContentLengthAttribute = (
Copy link
Member

Choose a reason for hiding this comment

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

What is the benefit of these functions?
It seems they are only called from tests but not from the plugin code itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are called just under by getOutgoingRequestAttributesOnResponse and getIncomingRequestAttributes

function getContentLength(
headers: OutgoingHttpHeaders | IncomingHttpHeaders
): number | null {
const contentLengthHeader = headers['content-length'];
Copy link
Member

Choose a reason for hiding this comment

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

At least for outgoing HTTP users may specify headers with any casing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

You refer to the object used by node http internally on symbol kOutHeaders accessible via the apis setHeader/ getHeader/... on outgoing requests.
But actually this is not applicable here. outgoing messages have not .header property at all.

But it seems this is no problem because this plugin captures content-size only for server requests (incoming) and client response (also incoming) where node parses and prepares the lowercased headers.

Co-authored-by: Gerhard Stöbich <[email protected]>
@dyladan
Copy link
Member

dyladan commented Dec 21, 2020

@vmarchaud do @Flarna's fixes need to be applied to the plugin version too?

@vmarchaud
Copy link
Member Author

@vmarchaud do @Flarna's fixes need to be applied to the plugin version too?

No the original PR for the http plugin have the correct version, i'm not sure why my cherry-pick resulted in this. Even more surprised than neither typescript or tests had issues with this.

@obecny obecny added the enhancement New feature or request label Dec 21, 2020
@dyladan dyladan merged commit d77a8b9 into open-telemetry:master Dec 22, 2020
pichlermarc added a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
martinkuba pushed a commit to martinkuba/opentelemetry-js that referenced this pull request Mar 13, 2024
martinkuba pushed a commit to martinkuba/opentelemetry-js that referenced this pull request Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add HTTP semantic convention for content size to http instrumentation
4 participants