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

ASGI: Capture custom request response headers #1004

Conversation

sanketmehta28
Copy link
Member

Description

This PR will add a feature to add custom request and response headers of http as well as websocket application to span as attributes.

Fixes # #919

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

I have added a new sample http_app_with_custom_headers and new websocket_app_with_custom_headers which will gives us sample apps to test and I have created a test class which will check expected as well as not expected span attributes in both type of applications

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

@sanketmehta28 sanketmehta28 requested a review from a team March 17, 2022 11:45
@@ -342,6 +384,13 @@ async def __call__(self, scope, receive, send):
for key, value in attributes.items():
current_span.set_attribute(key, value)

if span.kind == trace.SpanKind.SERVER:
Copy link
Member

@ashu658 ashu658 Mar 17, 2022

Choose a reason for hiding this comment

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

I guess if current_span.kind == trace.SpanKind.SERVER: should be used here.

@@ -395,6 +444,17 @@ async def otel_send(message):
set_status_code(server_span, 200)
set_status_code(send_span, 200)
send_span.set_attribute("type", message["type"])
if (
server_span.kind == trace.SpanKind.SERVER
Copy link
Member

Choose a reason for hiding this comment

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

We should check if server_span is a recording span before checking its kind. NonRecordingSpan do not contain kind attribute. (#998 was reported for wsgi based frameworks for the same)

return attributes


def collect_custom_response_headers_attributes(message):
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be ok, but please make sure any new public (non-underscore prefixed) function or symbol is really necessary and meant to be used directly by the user. If not, please prefix them with underscores.

Copy link
Member Author

@sanketmehta28 sanketmehta28 Mar 24, 2022

Choose a reason for hiding this comment

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

I understand. This functions will be used by other frameworks like django or FastAPI to fetch the custom headers and add them to spans as attributes. (just like collect_request_attributes/add_response_attributes)

sanketmehta28 and others added 3 commits March 24, 2022 22:28
…ure the span is recording before adding the attributes to span.

- changed span to current_span to make sure attributes are being added to proper span.
@sanketmehta28
Copy link
Member Author

sanketmehta28 commented Mar 25, 2022

@ocelotl @codeboten @lzchen Please do review and provide your approval on this PR.

@ocelotl ocelotl merged commit b1bf8d4 into open-telemetry:main Mar 29, 2022
@sanketmehta28 sanketmehta28 changed the title code change to add custom http and websocket request and response hea… ASGI: Capture custom request response headers Apr 29, 2022
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