Add environment variables for capturing Http headers #2461

ashu658 · 2022-03-31T09:39:30Z

Changes

Add environment variables to be configured for capturing http headers.
This spec (HTTP request and response headers) describes how to capture the http request/response header and add them in the span. It does not specify the environment variables need to be configured to capture the headers. This change specifies these environment variables.

Env vars proposed in the PR are already supported in Java and Python sdks.

Related PR: #1898

Additional info:
One thing that I have seen in python is that headers provided by some frameworks/libraries (e.g. wsgi based frameworks like flask) are lowercase and - is replaced with _ which would mean user who configures
OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST="CusTOM_HEAder-1"
will be able to capture custom-header-1 from request headers for some frameworks/libraries whereas for some other frameworks he won't be able to capture this header as it does not change - to _.
(All python frameworks/libraries I saw provides case-insensitive headers but only some of them replace - with _)
Mentioning this info here as I am not sure if it might affect this spec or not.

linux-foundation-easycla · 2022-03-31T09:39:33Z

The committers listed above are authorized under a signed CLA.

✅ login: ashu658 / name: Ashutosh Goel (a3477ba)

reyang · 2022-04-04T16:46:58Z

specification/sdk-environment-variables.md

+
+See [HTTP request and response headers](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-request-and-response-headers)
+
+Environment variables for capturing http request and response headers:


Suggested change

Environment variables for capturing http request and response headers:

Environment variables for capturing HTTP request and response headers:

reyang · 2022-04-04T16:47:11Z

CHANGELOG.md

@@ -16,6 +16,8 @@ release.
  ([#2276](https://github.com/open-telemetry/opentelemetry-specification/pull/2276)).
 - Add documentation REQUIREMENT for adding attributes at span creation.
  ([#2383](https://github.com/open-telemetry/opentelemetry-specification/pull/2383)).
+- Add environment variables for capturing Http headers.


Suggested change

- Add environment variables for capturing Http headers.

- Add environment variables for capturing HTTP headers.

reyang · 2022-04-04T16:48:56Z

specification/sdk-environment-variables.md

@@ -263,3 +263,19 @@ To ensure consistent naming across projects, this specification recommends that
 ```
 OTEL_{LANGUAGE}_{FEATURE}
 ```
+
+## HTTP request and response headers


What's the Status of this section?

reyang

https://github.com/open-telemetry/opentelemetry-specification/pull/2461/files#r841949238

jmacd · 2022-04-04T17:11:14Z

specification/sdk-environment-variables.md

+
+| Name                                                      | Description                                                                                                                                         |
+|-----------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------|
+| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST  | A comma-separated list of HTTP header names. HTTP server instrumentations will capture HTTP request header values for all configured header names.  |


Are these header names case-sensitive? Can you add a sentence or two saying what implementations are required to do with these variables in explicit terms?

From [RFC 7230] Section 3.2. Header Fields

Each header field consists of a case-insensitive field name followed
by a colon (":"), optional leading whitespace, the field value, and
optional trailing whitespace.

emphasis mine

The spec which describes how to capture the HTTP headers https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-request-and-response-headers requires the captured header names to be normalised (lowercase, with - characters replaced by _) when added as span attributes. So, two different header fields (e.g. custom-header and custom_header) will have the same span attribute key when added as span attribute.
I am not sure if this would be okay. Please share your thoughts.

Looks like the proposal for that behavior was done there by @Oberon00 #1898 (comment)

The motivation for lowercasing is obvious enough, but it seems the motivation to transform - to _ was simply so that it would comply with the naming convention of our other semantic conventions? IMO the transformation was a mistake but one that it might be too late to rectify. In any case, configuring which headers are captured is an entirely different problem than how to represent them in OTLP. I still stand by my case that the - to _ conversion is confusing within this header spec. Lowercasing seems fine and an obvious enough way to comply with case insensitivity.

dyladan · 2022-04-05T14:57:50Z

One thing that I have seen in python is that headers provided by some frameworks/libraries (e.g. wsgi based frameworks like flask) are lowercase and - is replaced with _ which would mean user who configures
OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST="CusTOM_HEAder-1"
will be able to capture custom-header-1 from request headers for some frameworks/libraries whereas for some other frameworks he won't be able to capture this header as it does not change - to _.
(All python frameworks/libraries I saw provides case-insensitive headers but only some of them replace - with _)
Mentioning this info here as I am not sure if it might affect this spec or not.

This seems very Python-specific to me and depends heavily on the library being used and the idioms of the ecosystem. I think this would be considered very confusing by most non-python users.

ashu658 · 2022-04-05T15:55:46Z

This seems very Python-specific to me and depends heavily on the library being used and the idioms of the ecosystem. I think this would be considered very confusing by most non-python users.

@dyladan thanks for the feedback! I was not sure if this might affect the spec so, mentioned it here.

yurishkuro · 2022-04-05T16:06:53Z

@open-telemetry/technical-committee I just did a quick search and did not find any env vars of the patten OTEL_INSTRUMENTATION_*. Which is understandable because so far the env vars were limited to configuring the SDKs. Capturing headers is not an SDK function, it's an instrumentation function. We have already heard concerns with having too many env vars that need to be supported by all SDKs. Do we want to extend this even further to configuring instrumentation libraries?

dyladan · 2022-04-05T16:14:22Z

@open-telemetry/technical-committee I just did a quick search and did not find any env vars of the patten OTEL_INSTRUMENTATION_*. Which is understandable because so far the env vars were limited to configuring the SDKs. Capturing headers is not an SDK function, it's an instrumentation function. We have already heard concerns with having too many env vars that need to be supported by all SDKs. Do we want to extend this even further to configuring instrumentation libraries?

Discussed this briefly on the spec call today and I think the consensus was that instrumentation-specific configuration doesn't belong in the SDK spec. The greater takeaway was the need for a consistent configuration specification and mechanism to ensure we can provide these configurations and others to all instrumentations in a consistent manner and document them appropriately. I can't remember who exactly but I think someone volunteered to get the ball rolling in that area.

svrnm · 2022-04-05T18:21:01Z

Discussed this briefly on the spec call today and I think the consensus was that instrumentation-specific configuration doesn't belong in the SDK spec. The greater takeaway was the need for a consistent configuration specification and mechanism to ensure we can provide these configurations and others to all instrumentations in a consistent manner and document them appropriately. I can't remember who exactly but I think someone volunteered to get the ball rolling in that area.

Just for our understanding: Does this mean we should close this PR and wait for the configuration specification & mechanism to come? Assuming that this will take some time, what to do with this in the meantime, Java and Python are using it already and if other languages jump on that same topic, it would be good to have used the same variables until the config exists.

tigrannajaryan · 2022-04-05T21:34:07Z

Just for our understanding: Does this mean we should close this PR and wait for the configuration specification & mechanism to come? Assuming that this will take some time, what to do with this in the meantime, Java and Python are using it already and if other languages jump on that same topic, it would be good to have used the same variables until the config exists.

I think we need to at least figure out some answers to these general questions so that all instrumentations follow a consistent configuration approach:

Should instrumentations be configurable via env variables?
Is the same configuration settable from the code?
Is naming of instrumentations' env variables dictated by the spec?
Where in the spec it should live?
What is the variable naming scheme? Do we choose a particular prefix like this PR suggests?
How to name variables which are specific for just one instrumentation and are not generic and applicable to many instrumentations like "OTEL_INSTRUMENTATION_HTTP*" is? Should the variable names be prefixed by the instrumentation library name?

This should be ideally opened as a separate issue, discussed and after we come to an agreement on what we want to do generally, we can unblock this PR.

svrnm · 2022-04-06T09:37:46Z

thanks @tigrannajaryan for the clarification, is #1773 the right issue to continue the conversation or is a new one doing a better job?

tigrannajaryan · 2022-04-06T14:27:22Z

thanks @tigrannajaryan for the clarification, is #1773 the right issue to continue the conversation or is a new one doing a better job?

I think #1773 is related but a bit different, I would not wait for it since it may also take longer to resolve and will unnecessarily delay this particular task. I would advise to open a separate issue about how instrumentations should be configured via env variables. When #1773 is ready it should take care of instrumentations' configuration too.

github-actions · 2022-04-14T03:27:27Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2022-04-21T03:43:12Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

ashu658 requested review from a team March 31, 2022 09:39

github-actions bot assigned arminru Mar 31, 2022

Adding environment vars to capture Http headers

a3477ba

ashu658 force-pushed the add-env-var-http-headers branch from ec4af0f to a3477ba Compare March 31, 2022 09:54

ashu658 changed the title ~~Adding environment variables for capturing Http headers~~ Add environment variables for capturing Http headers Mar 31, 2022

Updating CHANGELOG.md

e4e95fa

ashu658 mentioned this pull request Mar 31, 2022

Capture HTTP request/response headers as span attributes open-telemetry/opentelemetry-python-contrib#906

Closed

reyang reviewed Apr 4, 2022

View reviewed changes

reyang requested changes Apr 4, 2022

View reviewed changes

jmacd reviewed Apr 4, 2022

View reviewed changes

arminru added spec:trace Related to the specification/trace directory area:configuration Related to configuring the SDK labels Apr 5, 2022

This was referenced Apr 6, 2022

Configuration for instrumentations through environment variables #2472

Closed

Define best practices for no-code configuration #1773

Open

github-actions bot added the Stale label Apr 14, 2022

github-actions bot closed this Apr 21, 2022

trask mentioned this pull request Nov 9, 2022

Add gRPC request metadata instrumentation open-telemetry/opentelemetry-java-instrumentation#7011

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add environment variables for capturing Http headers #2461

Add environment variables for capturing Http headers #2461

ashu658 commented Mar 31, 2022 •

edited

Loading

linux-foundation-easycla bot commented Mar 31, 2022 •

edited

Loading

reyang Apr 4, 2022

reyang Apr 4, 2022

reyang Apr 4, 2022

reyang left a comment

jmacd Apr 4, 2022

dyladan Apr 5, 2022 •

edited

Loading

ashu658 Apr 5, 2022

dyladan Apr 5, 2022

dyladan commented Apr 5, 2022

ashu658 commented Apr 5, 2022 •

edited

Loading

yurishkuro commented Apr 5, 2022

dyladan commented Apr 5, 2022

svrnm commented Apr 5, 2022

tigrannajaryan commented Apr 5, 2022

svrnm commented Apr 6, 2022

tigrannajaryan commented Apr 6, 2022

github-actions bot commented Apr 14, 2022

github-actions bot commented Apr 21, 2022


		See [HTTP request and response headers](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-request-and-response-headers)

		Environment variables for capturing http request and response headers:

	- Add environment variables for capturing Http headers.
	- Add environment variables for capturing HTTP headers.

Add environment variables for capturing Http headers #2461

Add environment variables for capturing Http headers #2461

Conversation

ashu658 commented Mar 31, 2022 • edited Loading

Changes

linux-foundation-easycla bot commented Mar 31, 2022 • edited Loading

reyang Apr 4, 2022

Choose a reason for hiding this comment

reyang Apr 4, 2022

Choose a reason for hiding this comment

reyang Apr 4, 2022

Choose a reason for hiding this comment

reyang left a comment

Choose a reason for hiding this comment

jmacd Apr 4, 2022

Choose a reason for hiding this comment

dyladan Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

ashu658 Apr 5, 2022

Choose a reason for hiding this comment

dyladan Apr 5, 2022

Choose a reason for hiding this comment

dyladan commented Apr 5, 2022

ashu658 commented Apr 5, 2022 • edited Loading

yurishkuro commented Apr 5, 2022

dyladan commented Apr 5, 2022

svrnm commented Apr 5, 2022

tigrannajaryan commented Apr 5, 2022

svrnm commented Apr 6, 2022

tigrannajaryan commented Apr 6, 2022

github-actions bot commented Apr 14, 2022

github-actions bot commented Apr 21, 2022

ashu658 commented Mar 31, 2022 •

edited

Loading

linux-foundation-easycla bot commented Mar 31, 2022 •

edited

Loading

dyladan Apr 5, 2022 •

edited

Loading

ashu658 commented Apr 5, 2022 •

edited

Loading