Capture custom request/response headers for wsgi and change in passing response_headers in django, pyramid #925

ashu658 · 2022-02-17T09:50:44Z

Description

This PR implements the HTTP request/response headers as span attributes semantic convention for wsgi frameworks.
User can configure environment variable OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST and OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE to capture request/response headers respectively.
Changes django Instrumentation to pass response headers instead of response object.
Changes pyramid instrumentation to pass response headers as list.

Fixes #918

Type of change

Please delete options that are not relevant.

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 captured headers manually with wsgi based framework by observing the output on jaeger backend.

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

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py

sanketmehta28 · 2022-02-22T18:58:03Z

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py


    return result


+def capture_custom_request_headers(environ, attributes):


Any specific reason to pass the attribute dict as a argument and modifying it with new headers rather than returning the headers to calling function?
I think it will be better from readability perspective to update the result dict in a single function rather than passing to called function and update there

Agree. It is not clear what the function does without reading the function body.

Added suggested changes to improve readability. Please have a look.

sanketmehta28 · 2022-02-22T18:58:32Z

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

+            attributes[key] = [header_values]
+
+
+def capture_custom_response_headers(response_headers, span):


same comment as I have given in capture_custom_request_headers()

CHANGELOG.md

lzchen · 2022-02-22T19:26:09Z

util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py

 from urllib.parse import urlparse, urlunparse

+OTEL_CAPTURE_REQUEST_HEADERS = "OTEL_CAPTURE_REQUEST_HEADERS"


Do we want to further split this to client/server like Java does?

I think it is good to split as this will make the feature consistent between different sdks. Also, will keep the env vars same as java sdk.

Not sure but, when I first read the spec it felt like its only talking about the server side. Please share your thoughts :)

Seems like we are going with the general approach for being consistent with other language sdks like here so it would make sense to follow suit.

owais · 2022-02-22T23:48:46Z

...opentelemetry-instrumentation-pyramid/src/opentelemetry/instrumentation/pyramid/callbacks.py

@@ -161,7 +161,7 @@ def trace_tween(request):
                otel_wsgi.add_response_attributes(
                    span,
                    response_or_exception.status,
-                    response_or_exception.headers,
+                    response_or_exception.headerlist,


Is this safe for all supported version of django?

I guess you are referring to response.items() in .../instrumentation/django/middleware.py
Yes, I have tested it with lowest supported version of django by running the unit tests (as docs for django dont' say anything about this property below 4.0) and also done same for pyramid.
Do we run tests for different versions of a framework in the build jobs?

I was planning to add tests for all wsgi based frameworks in different PR.

I was referring to replacing headers with headerlist. Is headerlist attribute available on all supported versions of django?

available on all supported versions of django?

I think there is some confusion, the change header to headerlist is for pyramid :)

Is headerlist attribute available on all supported versions

Yes, I checked pyramid docs, headerlist attribute is available in all versions of pyramid we support (>=1.7).

owais · 2022-02-22T23:51:30Z

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py


    return result


+def capture_custom_request_headers(environ, attributes):


Agree. It is not clear what the function does without reading the function body.

- Django: Passing response header instead of response object

Fixing lint errors

Co-authored-by: Srikanth Chekuri <[email protected]>

1) Passing mulitple header values as single string in a list 2) Changing Env varibale name

Co-authored-by: Leighton Chen <[email protected]>

ashu658 · 2022-03-02T09:12:17Z

Can someone run combine benchmarks from previous build again?

lzchen · 2022-03-02T22:11:19Z

@sanketmehta28
Have your comments been addressed?

sanketmehta28 · 2022-03-03T06:26:30Z

@lzchen:yes I have checked it. They are addressed. I will approve this PR.

sanketmehta28 · 2022-03-03T06:26:30Z

@lzchen:yes I have checked it. They are addressed. I will approve this PR.

ashu658 · 2022-03-03T08:45:08Z

I forgot to add the condition to check if the span is a SERVER span before adding custom headers. Had to do some refactoring to add this condition. Please have a look. Apologies to the reviewers for late changes. 😓

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

srikanthccv

mostly LGTM; few questions.

srikanthccv · 2022-03-04T11:31:03Z

instrumentation/opentelemetry-instrumentation-wsgi/tests/test_wsgi_middleware.py

+            "http.response.header.content_type": (
+                "text/plain; charset=utf-8",
+            ),
+            "http.response.header.content_length": ("100",),


Hmm, why is everything a sequence? it doesn't make sense that content_length is list, right?

This is part of the spec actually.

The attribute value MUST consist of either multiple header values as an array of strings or a single-item array containing a possibly comma-concatenated string, depending on the way the HTTP library provides access to headers.

but that is for the multiple header values?

I think its for single values also. There is an example in the spec.

http.request.header.content_type=["application/json"]

That's strange but not a blocker for me.

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

ashu658 requested a review from a team February 17, 2022 09:50

ashu658 changed the title ~~Capture custom header wsgi~~ [WIP] Capture custom request/response headers for wsgi Feb 17, 2022

ashu658 marked this pull request as draft February 17, 2022 09:51

ashu658 force-pushed the capture-custom-header-wsgi branch from 3adf3de to e864f36 Compare February 17, 2022 10:22

ashu658 changed the title ~~[WIP] Capture custom request/response headers for wsgi~~ [WIP] Capture custom request/response headers for wsgi and correctly passing response_headers in django Feb 17, 2022

ashu658 changed the title ~~[WIP] Capture custom request/response headers for wsgi and correctly passing response_headers in django~~ [WIP] Capture custom request/response headers for wsgi and correctly passing response_headers in django, pyramid Feb 17, 2022

ashu658 force-pushed the capture-custom-header-wsgi branch from 447533e to f2b2288 Compare February 17, 2022 14:14

ashu658 marked this pull request as ready for review February 17, 2022 14:25

ashu658 changed the title ~~[WIP] Capture custom request/response headers for wsgi and correctly passing response_headers in django, pyramid~~ Capture custom request/response headers for wsgi and correctly passing response_headers in django, pyramid Feb 17, 2022

ashu658 changed the title ~~Capture custom request/response headers for wsgi and correctly passing response_headers in django, pyramid~~ Capture custom request/response headers for wsgi and change in passing response_headers in django, pyramid Feb 17, 2022

srikanthccv reviewed Feb 18, 2022

View reviewed changes

sanketmehta28 requested changes Feb 22, 2022

View reviewed changes

lzchen reviewed Feb 22, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lzchen reviewed Feb 22, 2022

View reviewed changes

owais reviewed Feb 22, 2022

View reviewed changes

Ashutosh Goel and others added 14 commits February 24, 2022 18:12

Capture custom request/response headers for wsgi

10fd32f

Adding unit tests and refactoring

12e5951

Formatting changes for fixing lint errors

bd75167

-Adding changelog

d706f6f

- Django: Passing response header instead of response object

Fixing lint errors

059b9be

Formatting correction

63ec660

Passing response headers in correct format in pyramid

e18dc94

Fixing lint errors

Resolving review comment

0541ecc

Co-authored-by: Srikanth Chekuri <[email protected]>

Adding type hints

88249d2

Fixing build errors

93fa969

resolving review comments:

191f1a7

1) Passing mulitple header values as single string in a list 2) Changing Env varibale name

Fixing unit tests

9bd0864

Fixing lint errors

5878aa0

Update CHANGELOG.md

aedc4e2

Co-authored-by: Leighton Chen <[email protected]>

ashu658 force-pushed the capture-custom-header-wsgi branch from 8011ad5 to aedc4e2 Compare February 24, 2022 12:42

lzchen approved these changes Mar 1, 2022

View reviewed changes

ashu658 requested review from srikanthccv and owais March 2, 2022 11:03

Merge branch 'main' into capture-custom-header-wsgi

f95859d

sanketmehta28 approved these changes Mar 3, 2022

View reviewed changes

ashu658 changed the title ~~Capture custom request/response headers for wsgi and change in passing response_headers in django, pyramid~~ [WIP] Capture custom request/response headers for wsgi and change in passing response_headers in django, pyramid Mar 3, 2022

Adding custom header only if span is SERVER span

df33267

ashu658 requested a review from lzchen March 3, 2022 08:45

Fixing lint errors

bd24673

ashu658 changed the title ~~[WIP] Capture custom request/response headers for wsgi and change in passing response_headers in django, pyramid~~ Capture custom request/response headers for wsgi and change in passing response_headers in django, pyramid Mar 3, 2022

Removing unused variables

d85650e

sanketmehta28 requested changes Mar 3, 2022

View reviewed changes

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py Outdated Show resolved Hide resolved

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py Outdated Show resolved Hide resolved

Ashutosh Goel added 2 commits March 3, 2022 20:10

Addressing comments

98ad58b

Fixing pytests

f9e40a3

sanketmehta28 approved these changes Mar 3, 2022

View reviewed changes

srikanthccv reviewed Mar 4, 2022

View reviewed changes

srikanthccv approved these changes Mar 4, 2022

View reviewed changes

sanketmehta28 mentioned this pull request Mar 7, 2022

REQUEST: New membership for sanketmehta28 open-telemetry/community#995

Closed

6 tasks

Merge branch 'main' into capture-custom-header-wsgi

c4a5998

lzchen approved these changes Mar 7, 2022

View reviewed changes

lzchen merged commit 2f5bbc4 into open-telemetry:main Mar 7, 2022

ashu658 deleted the capture-custom-header-wsgi branch March 8, 2022 05:49

ashu658 mentioned this pull request Mar 8, 2022

REQUEST: New membership for ashu658 open-telemetry/community#998

Closed

6 tasks

ashu658 mentioned this pull request Apr 18, 2022

Issue to track Ashu's contributions for contrib approver #1054

Closed

sanketmehta28 mentioned this pull request Apr 19, 2022

Issue to track Sanket's work for contrib approver #1053

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capture custom request/response headers for wsgi and change in passing response_headers in django, pyramid #925

Capture custom request/response headers for wsgi and change in passing response_headers in django, pyramid #925

ashu658 commented Feb 17, 2022 •

edited

Loading

sanketmehta28 Feb 22, 2022

owais Feb 22, 2022

ashu658 Feb 24, 2022

sanketmehta28 Feb 22, 2022

lzchen Feb 22, 2022

ashu658 Feb 24, 2022

lzchen Feb 28, 2022

owais Feb 22, 2022

ashu658 Feb 24, 2022

ashu658 Feb 24, 2022

owais Mar 2, 2022

ashu658 Mar 3, 2022 •

edited

Loading

owais Feb 22, 2022

ashu658 commented Mar 2, 2022

lzchen commented Mar 2, 2022

sanketmehta28 commented Mar 3, 2022

sanketmehta28 commented Mar 3, 2022

ashu658 commented Mar 3, 2022 •

edited

Loading

srikanthccv left a comment

srikanthccv Mar 4, 2022

ashu658 Mar 4, 2022

srikanthccv Mar 4, 2022

ashu658 Mar 4, 2022

srikanthccv Mar 4, 2022 •

edited

Loading


		return result


		def capture_custom_request_headers(environ, attributes):

		attributes[key] = [header_values]


		def capture_custom_response_headers(response_headers, span):

		from urllib.parse import urlparse, urlunparse

		OTEL_CAPTURE_REQUEST_HEADERS = "OTEL_CAPTURE_REQUEST_HEADERS"

Capture custom request/response headers for wsgi and change in passing response_headers in django, pyramid #925

Capture custom request/response headers for wsgi and change in passing response_headers in django, pyramid #925

Conversation

ashu658 commented Feb 17, 2022 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashu658 Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashu658 commented Mar 2, 2022

lzchen commented Mar 2, 2022

sanketmehta28 commented Mar 3, 2022

sanketmehta28 commented Mar 3, 2022

ashu658 commented Mar 3, 2022 • edited Loading

srikanthccv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikanthccv Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

ashu658 commented Feb 17, 2022 •

edited

Loading

ashu658 Mar 3, 2022 •

edited

Loading

ashu658 commented Mar 3, 2022 •

edited

Loading

srikanthccv Mar 4, 2022 •

edited

Loading