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.

Sign up for GitHub

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

Change metadata lookup to be case insensitive #5646

Merged
merged 2 commits into from
Jul 13, 2022
Merged

Change metadata lookup to be case insensitive #5646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9ee728
Change metadata lookup to be case insensitive
jpkrohling Jul 7, 2022
cdefedb
Add changelog entry
jpkrohling Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### 💡 Enhancements 💡

- Add `linux-ppc64le` architecture to cross build tests in CI
- `client`: perform case insensitive lookups in case the requested metadata value isn't found (#5646)

## v0.55.0 Beta

Expand Down
15 changes: 14 additions & 1 deletion client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ package client // import "go.opentelemetry.io/collector/client"
import (
"context"
"net"
"strings"
)

type ctxKey struct{}
Expand Down Expand Up @@ -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?
Copy link
Member

@mx-psi mx-psi Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for k, v := range m.data {
if strings.EqualFold(key, k) {
vals = v
// we optimize for the next lookup
m.data[key] = v
}
}

// if it's still not found, it's really not here
if len(vals) == 0 {
return nil
}
}

ret := make([]string, len(vals))
Expand Down
1 change: 1 addition & 0 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ func TestMetadata(t *testing.T) {
source := map[string][]string{"test-key": {"test-val"}}
md := NewMetadata(source)
assert.Equal(t, []string{"test-val"}, md.Get("test-key"))
assert.Equal(t, []string{"test-val"}, md.Get("test-KEY")) // case insensitive lookup

// test if copy. In regular use, source cannot change
val := md.Get("test-key")
Expand Down