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

Normalizing HTTP header names is ambiguous #304

Closed
Flarna opened this issue Sep 6, 2023 · 5 comments · Fixed by #369
Closed

Normalizing HTTP header names is ambiguous #304

Flarna opened this issue Sep 6, 2023 · 5 comments · Fixed by #369
Assignees

Comments

@Flarna
Copy link
Member

Flarna commented Sep 6, 2023

HTTP Span SemConv requires to normalize HTTP header name attributes.

using lower case is fine because HTTP tells to handle header names case insensitive (HTTP/2 requires to them even lowercase).

But replacing - by _ seems problematic because both chars are allowed in http header names.

Therefore this may result in merging/overwrite issues on instrumentation side and unclear interpretation on backend side.

@Oberon00
Copy link
Member

Oberon00 commented Sep 6, 2023

Probably we should define which one wins in this case. I'd go for the hyphen one, as the underscore one does not usually occur and could be used by a bad actor to confuse tracing. Although header names to capture are anyways configured one-by-one IIRC, so it should not be a security problem

@Flarna
Copy link
Member Author

Flarna commented Sep 6, 2023

I would be in favor of removing this part of the normalization but I fear that ship has sailed or would at least requires a 2.0 release.

@Oberon00
Copy link
Member

Oberon00 commented Sep 6, 2023

This normalization (or rather the reversion of it) is very common, and also done by CGI and Python WSGI, so I don't think there will be a problem in practice.

@trask
Copy link
Member

trask commented Oct 3, 2023

I would be in favor of removing this part of the normalization but I fear that ship has sailed or would at least requires a 2.0 release.

It's not too late yet, but soon will be. We will be marking HTTP semantic conventions as stable very soon.

Removing the - to _ part of the normalization seems reasonable to me, and reporting attributes like:

  • http.request.header.content-type=["application/json"]
  • http.request.header.x-forwarded-for=["1.2.3.4", "1.2.3.5"]

instead of

  • http.request.header.content_type=["application/json"]
  • http.request.header.x_forwarded_for=["1.2.3.4", "1.2.3.5"]

@open-telemetry/specs-semconv-approvers do we have a reason to try to avoid dashes (-) in attribute names?

I don't see discussion/context of why we would want to do this particular part of the normalization when it was originally proposed in open-telemetry/opentelemetry-specification#1898 (comment)

@trask
Copy link
Member

trask commented Oct 3, 2023

sent #369 to help attract attention and force a decision one way or another on this issue before stabilization

@joaopgrassi joaopgrassi moved this from Blocker for stability to In Progress in Spec: HTTP Semantic Conventions Oct 16, 2023
@joaopgrassi joaopgrassi moved this from In Progress to Blocker for stability in Spec: HTTP Semantic Conventions Oct 16, 2023
@github-project-automation github-project-automation bot moved this from Blocker for stability to Done in Spec: HTTP Semantic Conventions Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
4 participants