Change metadata lookup to be case insensitive #5646
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5646 +/- ##
==========================================
- Coverage 91.33% 91.30% -0.03%
==========================================
Files 191 191
Lines 11384 11392 +8
==========================================
+ Hits 10398 10402 +4
- Misses 786 789 +3
- Partials 200 201 +1
Continue to review full report at Codecov.
|
jpkrohling
force-pushed
the
jpkrohling/issue5610
branch
from
f0962c5 to
23eda18
Compare
|
@TylerHelmuth The RFC is very prescriptive about this: https://www.rfc-editor.org/rfc/rfc9110.html#section-5.1 Therefore, I'd rather avoid any support for case-sentitive usage |
| @@ -160,7 +161,19 @@ func NewMetadata(md map[string][]string) Metadata { | |||
| func (m Metadata) Get(key string) []string { | |||
| vals := m.data[key] | |||
| if len(vals) == 0 { | |||
| return nil | |||
| // we didn't find the key, but perhaps it just has different cases? | |||
There was a problem hiding this comment.
Have you explored the possibility of making keys lowercase on NewMetadata? I am not sure how this API is used, so there may be performance implications, but it would make the code much simpler
There was a problem hiding this comment.
Yes, but given this is on the hot path, I didn't want to incur extra processing when not absolutely needed. I expect the typical case to still hit the old code path, as users would likely use the same case as the header.
Before this change, users would make references to headers such as "authorization" and "X-CloudFront-Viewer-Latitude", which would potentially fail, as they would be stored in the context as "X-Cloudfront-Viewer-Latitude" or "Authorization". This PR changes this behavior, to attempt a case insensitive lookup to the backing map in case the key wasn't found under the requested casing. Under the assumption that the lookup key specified by users won't change, on a subsequent lookup, we proactively copy the value to the looked up key, to make the next lookup faster. Fixes open-telemetry#5610 Fixes open-telemetry/opentelemetry-collector-contrib#8994 Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling
force-pushed
the
jpkrohling/issue5610
branch
from
23eda18 to
cdefedb
Compare
|
@codeboten, I believe this is ready to be merged |
mx-psi
added
the
ready-to-merge
Before this change, users would make references to headers such as "authorization" and "X-CloudFront-Viewer-Latitude", which would potentially fail, as they would be stored in the context as "X-Cloudfront-Viewer-Latitude" or "Authorization".
This PR changes this behavior, to attempt a case insensitive lookup to the backing map in case the key wasn't found under the requested casing. Under the assumption that the lookup key specified by users won't change, on a subsequent lookup, we proactively copy the value to the looked up key, to make the next lookup faster.
Fixes #5610
Fixes open-telemetry/opentelemetry-collector-contrib#8994
Signed-off-by: Juraci Paixão Kröhling [email protected]