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 the option to capture headers as span attributes #2492

Merged
merged 9 commits into from
Sep 27, 2021

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Sep 23, 2021

Which problem is this PR solving?

Adds the option to the HTTP instrumentation to capture headers as span attributes.
Relevant section in the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-request-and-response-headers

The headers which need capturing need to be set explicitly, by default no headers are transformed to span attributes.

For example if X-Forwarded-For is set to be captured on request headers, the span will have http.request.header.x_forwarded_for attribute set.

@seemk seemk requested a review from a team September 23, 2021 08:43
@codecov
Copy link

codecov bot commented Sep 23, 2021

Codecov Report

Merging #2492 (d2c8c61) into main (33935e7) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2492      +/-   ##
==========================================
+ Coverage   93.20%   93.23%   +0.03%     
==========================================
  Files         137      137              
  Lines        5017     5043      +26     
  Branches     1060     1067       +7     
==========================================
+ Hits         4676     4702      +26     
  Misses        341      341              
Impacted Files Coverage Δ
...es/opentelemetry-instrumentation-http/src/utils.ts 99.08% <0.00%> (+0.05%) ⬆️
...ges/opentelemetry-instrumentation-http/src/http.ts 94.90% <0.00%> (+0.25%) ⬆️

@Flarna
Copy link
Member

Flarna commented Sep 24, 2021

Please update the readme with documentation of this new option.

});
instrumentation.enable();
server = http.createServer((request, response) => {
response.setHeader('X-Server-Header1', 'server123');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
response.setHeader('X-Server-Header1', 'server123');
response.setHeader('X-ServEr-hEeader1', 'server123');

or similar to verify that casing doesn't matter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, added it!

@seemk
Copy link
Contributor Author

seemk commented Sep 24, 2021

Please update the readme with documentation of this new option.

Updated the README

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.

6 participants