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

Pyramid: Capture custom request/response headers #1022

Merged

Conversation

ashu658
Copy link
Member

@ashu658 ashu658 commented Mar 29, 2022

Description

This PR implements the HTTP request/response headers as span attributes semantic convention for pyramid framework.

Fixes #914

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Added unit tests for the changes. Also, tested by capturing headers manually with instrumented pyramid app and observing the output on jaeger backend.
Added unit test for non-recording span.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@ashu658 ashu658 requested a review from a team March 29, 2022 06:18
@ashu658
Copy link
Member Author

ashu658 commented Mar 29, 2022

Can someone re-run the failing builds?

@ashu658
Copy link
Member Author

ashu658 commented Mar 30, 2022

I have some small questions.

  1. Should we add usage for each instrumentation or a single usage doc? As the env vars are framework independent.

  2. In http request headers provided by WSGI based frameworks - is replaced with _ (tested with pyramid, flask) . This would mean OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST="custom_request_header" will capture custom-request-header (even though header name are not same) whereas in asgi request headers provided by framework do not replace - with _ (for fastapi). Similarly, response headers can preserve casing and - is also not replaced with _.
    This creates some inconsistencies in capturing headers among different frameworks.
    I was thinking we should leave it to the user to provide correct name of the header to be captured. (although giving wrong headers names might be able to capture header for some framework e.g. wsgi frameworks). Please share your thoughts.
    cc @lzchen @srikanthccv @owais

@lzchen
Copy link
Contributor

lzchen commented Mar 30, 2022

@ashu658

For (1), we should probably stick with separate usage docs per instrumentation for the reason you listed above.
For (2), doesn't asgi instrumentation normalize the headers as well?

@ashu658
Copy link
Member Author

ashu658 commented Mar 30, 2022

@lzchen
I think there is some confusion.
yes, in asgi we normalise the header name to be added to the span attributes. This is same for both wsgi and asgi.
But I was talking about the difference when we are extracting the headers from the request. The request headers that we get from the framework is already normalised in case of wsgi frameworks (as we get wsgi headers as wsgi env vars where it replaces - with _) whereas in asgi it preserves -. When we try to match the header name to capture it there can be differences between wsgi and asgi.

Example, in case a user sends a request with custom-request-header. He will be able to capture it with OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST="custom_request_header" in wsgi but not in asgi (as asgi will preserve - and the header name won't match).
Hope its clear :)

@ashu658
Copy link
Member Author

ashu658 commented Mar 31, 2022

In 31 March SIG meet discussion it is decided to document the above mentioned behaviour as an example in instrumentation docs and also add a recommended way to configure these environment variables for each instrumentation.
Something like, in wsgi instrumentation docs we will mention that OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST="custom_request_header" can caputre custom-request-header and also the recommended way to configure the env vars.

will extract ``content-type`` and ``custom_response_header`` from response headers and add them as span attributes.

It is recommended that you should give the correct names of the headers to be captured in the environment variable.
Response header names captured in pyramid are case insensitive. So, giving header name as ``CUStomHeader`` in environment variable will be able capture header with name ``customheader``.
Copy link
Member Author

Choose a reason for hiding this comment

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

Request and response headers we get are different so, I have to give separate behaviour examples for request and response headers as the headers.

@srikanthccv
Copy link
Member

@ashu658 fix conflicts and address the unresolved comments.

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.

Pyramid: Capture request/response headers as span attributes
5 participants