Capture HTTP request/response headers as span attributes #1898

mateuszrzeszutek · 2021-08-30T17:32:45Z

Changes

Add semantic conventions for capturing HTTP request and response headers.

specification/trace/semantic_conventions/http.md

Oberon00

This convention is breaking new ground, because it only defines a prefix, not a full semantic convention. We don't really have support for that in the semantic convention generator (yet?), so it's fine to only have it as a markdown table.

There are a few questions that arise from this convention, that I'll post as separate comments.

specification/trace/semantic_conventions/http.md

jsuereth · 2021-08-31T15:19:38Z

Should HTTP Request/Response be an "Event" with these as attributes on that event?

yurishkuro · 2021-08-31T15:22:48Z

Should HTTP Request/Response be an "Event" with these as attributes on that event?

I'd say no, because there are no meaningful timestamps one could attach to such events that are different from the span start/end.

specification/trace/semantic_conventions/http.md

Oberon00 · 2021-09-07T11:53:28Z

After seeing #1907, I wonder if my proposal to preserve casing was so good after all, or if we need something additional. IMHO a good goal would be to be able to capture header information in such a way that #1907 is resolved as well. Some ideas:

Normalize header names to lowercase, - to _ (like lowercased CGI envvars), do not preserve casing.
Optionally capture a list of configured headers normalized (in addition to case-preserving or only). The prefix could be the same or http.(request|response).normalized_header or similar. Or we could rename the current capturing to raw_header. The normalization could be done by the instrumentation, a span processor (if we have Add BeforeEnd to have a callback where the span is still writeable #1089), the exporter, the collector or even the backend.
Just state that normalization / accessing keys in a case-insensitive way is the responsibility of whoever processes the data (maybe in addition to defining the normalization but define nothing more. I don't think that would be good enough to satisfy proposals like Capture request/response media type #1907 in the future though.

CC @Blacksmoke16

Blacksmoke16 · 2021-09-07T12:52:26Z

@Oberon00 HTTP/2 requires headers be lowercase while in HTTP/1 it doesn't matter. Normalizing them to all lowercase would be backwards compatible, while also being future proof.

mateuszrzeszutek · 2021-09-08T16:03:18Z

Normalize header names to lowercase, - to _ (like lowercased CGI envvars), do not preserve casing.

I like this idea because it's the simplest out of the three you suggested - and the most intuitive one ("why don't we lowercase headers" was the first question I asked in the linked issue 😄 ). We do lose a little bit of information that way, but since HTTP headers are supposed to be case insensitive by design it's probably not worth that much.
I'll rework my PR with idea 1. in mind.

specification/trace/semantic_conventions/http.md

carlosalberto · 2021-09-17T12:18:52Z

Why not have this as attributes with "type" map[string][]string

Since we don't support this now (and probably it will take a little while before we do so), should we proceed with this approach for now? @bogdandrutu @yurishkuro

Oberon00 · 2021-09-17T12:21:34Z

and probably it will take a little while before we do so

You are assuming that we will support it 😉 That's not decided at all.

carlosalberto · 2021-09-17T12:23:19Z

That's not decided at all.

Yeah, definitely. More the reason to go with the current approach.

Oberon00 · 2021-09-17T12:25:32Z

My thoughts: I'm somewhat strongly against adding support for more complex attributes, like maps. I think this PR should be merged as-is, and then we should close #376 because the only use case that came up so far has been solved differently. What would the purpose of adding maps after this PR be? The added-value of maps is already very small (negative IMHO), and if we add maps after this PR, there is one more negative to it, namely the disruption it causes for this semantic convention.

specification/trace/semantic_conventions/http.md

Oberon00

I'm against having the value be set as a single string instead of single-element array, as it makes typing more complicated. See #1898 (comment)

specification/trace/semantic_conventions/http.md

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

specification/trace/semantic_conventions/http.md

carlosalberto · 2021-09-20T10:54:17Z

Will merge by EOD unless somebody opposes.

mateuszrzeszutek · 2021-09-21T16:54:09Z

@open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers please take a look at this PR 🙏

linux-foundation-easycla · 2021-09-21T17:40:59Z

The committers are authorized under a signed CLA.

✅ Mateusz Rzeszutek (9bdf1e6, b11f7af, fb11220, 5b2f882, ad335c1, 19823de, d39e727, bfb35dd, 12151ec, 749a9cb)
✅ Yuri Shkuro (05e3149)

…try#1898) * Capture HTTP request/response headers as span attributes * Reference footnote [2] * code review suggestions * code review suggestions * code review comments * normalize HTTP header names * Update specification/trace/semantic_conventions/http.md Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> * code review suggestion * Add a CHANGELOG entry * code review suggestions Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

mateuszrzeszutek requested review from a team August 30, 2021 17:32

github-actions bot assigned arminru Aug 30, 2021

jkwatson reviewed Aug 30, 2021

View reviewed changes

specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved

jkwatson approved these changes Aug 30, 2021

View reviewed changes

Oberon00 reviewed Aug 31, 2021

View reviewed changes

specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved

specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved

arminru added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory labels Aug 31, 2021

Oberon00 reviewed Aug 31, 2021

View reviewed changes

specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved

Oberon00 approved these changes Aug 31, 2021

View reviewed changes

owais reviewed Sep 1, 2021

View reviewed changes

specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved

specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved

specification/trace/semantic_conventions/http.md Show resolved Hide resolved

Blacksmoke16 mentioned this pull request Sep 7, 2021

Capture request/response media type #1907

Closed

bogdandrutu reviewed Sep 9, 2021

View reviewed changes

specification/trace/semantic_conventions/http.md Show resolved Hide resolved

Oberon00 mentioned this pull request Sep 15, 2021

Validate that enum member IDs are valid open-telemetry/build-tools#64

Merged

carlosalberto approved these changes Sep 17, 2021

View reviewed changes

yurishkuro approved these changes Sep 17, 2021

View reviewed changes

specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved

Oberon00 requested changes Sep 17, 2021

View reviewed changes

arminru approved these changes Sep 17, 2021

View reviewed changes

specification/trace/semantic_conventions/http.md Show resolved Hide resolved

Oberon00 approved these changes Sep 17, 2021

View reviewed changes

Capture HTTP request/response headers as span attributes

9bdf1e6

Mateusz Rzeszutek and others added 8 commits September 17, 2021 15:37

Reference footnote [2]

b11f7af

code review suggestions

fb11220

code review suggestions

5b2f882

code review comments

ad335c1

normalize HTTP header names

19823de

Update specification/trace/semantic_conventions/http.md

d39e727

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

code review suggestion

bfb35dd

Add a CHANGELOG entry

12151ec

mateuszrzeszutek force-pushed the capture-http-headers branch from 7747d92 to 12151ec Compare September 17, 2021 13:37

Oberon00 reviewed Sep 17, 2021

View reviewed changes

specification/trace/semantic_conventions/http.md Outdated Show resolved Hide resolved

code review suggestions

749a9cb

yurishkuro enabled auto-merge (squash) September 21, 2021 17:40

Merge branch 'main' into capture-http-headers

05e3149

iNikem approved these changes Sep 21, 2021

View reviewed changes

Oberon00 approved these changes Sep 21, 2021

View reviewed changes

yurishkuro merged commit e0fb920 into open-telemetry:main Sep 21, 2021

mateuszrzeszutek deleted the capture-http-headers branch October 1, 2021 06:58

ashu658 mentioned this pull request Feb 8, 2022

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

Closed

This was referenced Mar 31, 2022

Adding environment variables for Http headers ashu658/opentelemetry-specification#1

Draft

Add environment variables for capturing Http headers #2461

Closed

trask mentioned this pull request Oct 3, 2023

Normalizing HTTP header names is ambiguous open-telemetry/semantic-conventions#304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capture HTTP request/response headers as span attributes #1898

Capture HTTP request/response headers as span attributes #1898

mateuszrzeszutek commented Aug 30, 2021

Oberon00 left a comment

jsuereth commented Aug 31, 2021

yurishkuro commented Aug 31, 2021

Oberon00 commented Sep 7, 2021

Blacksmoke16 commented Sep 7, 2021 •

edited

Loading

mateuszrzeszutek commented Sep 8, 2021

carlosalberto commented Sep 17, 2021

Oberon00 commented Sep 17, 2021

carlosalberto commented Sep 17, 2021

Oberon00 commented Sep 17, 2021 •

edited

Loading

Oberon00 left a comment •

edited

Loading

carlosalberto commented Sep 20, 2021

mateuszrzeszutek commented Sep 21, 2021

linux-foundation-easycla bot commented Sep 21, 2021 •

edited

Loading

Capture HTTP request/response headers as span attributes #1898

Capture HTTP request/response headers as span attributes #1898

Conversation

mateuszrzeszutek commented Aug 30, 2021

Changes

Oberon00 left a comment

Choose a reason for hiding this comment

jsuereth commented Aug 31, 2021

yurishkuro commented Aug 31, 2021

Oberon00 commented Sep 7, 2021

Blacksmoke16 commented Sep 7, 2021 • edited Loading

mateuszrzeszutek commented Sep 8, 2021

carlosalberto commented Sep 17, 2021

Oberon00 commented Sep 17, 2021

carlosalberto commented Sep 17, 2021

Oberon00 commented Sep 17, 2021 • edited Loading

Oberon00 left a comment • edited Loading

Choose a reason for hiding this comment

carlosalberto commented Sep 20, 2021

mateuszrzeszutek commented Sep 21, 2021

linux-foundation-easycla bot commented Sep 21, 2021 • edited Loading

Blacksmoke16 commented Sep 7, 2021 •

edited

Loading

Oberon00 commented Sep 17, 2021 •

edited

Loading

Oberon00 left a comment •

edited

Loading

linux-foundation-easycla bot commented Sep 21, 2021 •

edited

Loading