Skip to content

Fix HTTP header normalization#2260

Closed
samuelcolvin wants to merge 7 commits intoopen-telemetry:mainfrom
samuelcolvin:fix-http-header-normalization
Closed

Fix HTTP header normalization#2260
samuelcolvin wants to merge 7 commits intoopen-telemetry:mainfrom
samuelcolvin:fix-http-header-normalization

Conversation

@samuelcolvin
Copy link
Copy Markdown
Contributor

@samuelcolvin samuelcolvin commented Feb 23, 2024 •
edited
Loading

Description

HTTP request and response headers (attributes http.request.header.* and http.response.header.*) should be normalized by just being converted to lowercase, not replacing underscore with dash.

This is made clear in the docs here:

image image

You can also see the correct behavior in the go library here.

The current incorrect behavior of replace('_', '-') is extremely problematic since you can't know what the original header was - e.g. if you see a key http.request.header.foo_bar what was the original value? It could have been foo-bar or foo_bar.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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

  • replaced all the tests expecting the incorrect behavior

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • 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

@samuelcolvin samuelcolvin requested a review from a team February 23, 2024 12:58
@github-actions github-actions Bot requested review from ocelotl and shalevr February 23, 2024 12:58
federicobond
federicobond approved these changes Feb 23, 2024
View reviewed changes
Comment thread ...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Comment thread ...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
@aabmass
Copy link
Copy Markdown
Member

aabmass commented Feb 23, 2024

Thanks for the PR @samuelcolvin. I did some digging and it looks like the OTel conventions were changed in open-telemetry/semantic-conventions#369 but used to require this normalization. We should definitely fix it, but I think it might need to be gated behind the OTEL_SEMCONV_STABILITY_OPT_IN environment variable described here.

@lmolkova @lzchen could we merge this PR today or does it need to be controlled by OTEL_SEMCONV_STABILITY_OPT_IN? I'm not sure how we could emit http/dup if there is only one attribute key in question.

@samuelcolvin samuelcolvin mentioned this pull request Feb 23, 2024
Merged
11 tasks
@lzchen
Copy link
Copy Markdown
Contributor

lzchen commented Feb 23, 2024

@aabmass

This might be a bigger discussion on how we should be handling the new http semantic convention migrations. For now, please hold off on this PR. Will bring this up in the weekly SIG.

@samuelcolvin
Copy link
Copy Markdown
Contributor Author

samuelcolvin commented Feb 23, 2024 •
edited
Loading

@lzchen ok understood, can we at least run tests, so I know it's passing.

Also, if this won't get merged yet, could we merge #2266 instead first please? There's no "http semantic convention migrations" issue there, just a bug.

@aabmass
Copy link
Copy Markdown
Member

aabmass commented Feb 23, 2024

can we at least run tests, so I know it's passing.

Running now

Also, if this won't get merged yet, could we merge #2266 instead first please? There's no "http semantic convention migrations" issue there, just a bug.

That SGTM as it's just a bug 👍

@lzchen lzchen mentioned this pull request Mar 5, 2024
Closed
@federicobond federicobond mentioned this pull request Mar 14, 2024
Closed
6 tasks
@lzchen
Copy link
Copy Markdown
Contributor

lzchen commented Jul 11, 2024

From discussion of 07/11/2024 SIG, just an update on what is blocking this PR. Some of the instrumentations that this pr touches still has not been migrated to new semantic conventions as of yet, most notably django, falcon, pyramid, starlette and tornado. Since django is the only one remaining for the stable http semantic migration plan, once that instrumentation is migrated, we can go ahead with this change as we have agreed that it is fine to make breaking/semantic convention changes outright to the other instrumentations since they are all in beta.

@ocelotl ocelotl removed their assignment Sep 3, 2024
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 11, 2026

This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment.
If you're still working on this, please add a comment or push new commits.

@github-actions github-actions Bot added the Stale label Mar 11, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 25, 2026

This PR has been closed due to inactivity. Please reopen if you would like to continue working on it.

@github-actions github-actions Bot closed this Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ocelotl ocelotl Awaiting requested review from ocelotl

@shalevr shalevr Awaiting requested review from shalevr

1 more reviewer

@federicobond federicobond federicobond approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@shalevr shalevr

Labels

Stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@samuelcolvin @aabmass @lzchen @federicobond @ocelotl @shalevr