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

improvement: Drop keyValue field in metrics.Tag; normalizeTag short-circuits simple strings #324

Merged
merged 2 commits into from
Sep 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions metrics/benchmark_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,19 @@ func BenchmarkNewTag(b *testing.B) {
}
}

func BenchmarkNormalizeTag(b *testing.B) {
for _, val := range []string{
"a❌Long❌Tag❌With❌Emoji❌Chars",
"a Long Tag With Space Chars",
"aLongTagValueWithUpperChars",
"alongtagvaluewithnospecials",
"UPPER",
"lower",
} {
b.Run(val, newTagBenchFunc("key", val))
}
}

func newTagBenchFunc(tagKey, tagValue string) func(*testing.B) {
return func(b *testing.B) {
b.ReportAllocs()
Expand Down
8 changes: 5 additions & 3 deletions metrics/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ func (r *rootRegistry) Subregistry(prefix string, tags ...Tag) Registry {
}
return &childRegistry{
prefix: prefix,
tags: Tags(tags),
tags: tags,
root: r,
}
}
Expand Down Expand Up @@ -385,14 +385,16 @@ func toMetricTagsID(name string, tags Tags) metricTagsID {
// calculate how large to make our byte buffer below
bufSize := len(name)
for _, t := range tags {
bufSize += len(t.keyValue) + 1 // 1 for separator
bufSize += len(t.key) + len(t.value) + 2 // 2 for separators
}
buf := strings.Builder{}
buf.Grow(bufSize)
_, _ = buf.WriteString(name)
for _, tag := range tags {
_, _ = buf.WriteRune('|')
_, _ = buf.WriteString(tag.keyValue)
_, _ = buf.WriteString(tag.key)
_, _ = buf.WriteRune(':')
_, _ = buf.WriteString(tag.value)
}
return metricTagsID(buf.String())
}
Expand Down
12 changes: 8 additions & 4 deletions metrics/sort_go121.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,15 @@ func sortTags(tags Tags) {

func compareTags(a, b Tag) int {
switch {
case a.keyValue > b.keyValue:
case a.key < b.key:
return -1
case a.key > b.key:
return 1
case a.keyValue == b.keyValue:
return 0
default:
case a.value < b.value:
return -1
case a.value > b.value:
return 1
default:
return 0
}
}
25 changes: 18 additions & 7 deletions metrics/tag.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ import (
type Tag struct {
key string
value string
// Store the concatenated key and value so we don't need to reconstruct it in String() (used in toMetricTagID)
keyValue string
}

func (t Tag) Key() string {
Expand All @@ -32,7 +30,7 @@ func (t Tag) Value() string {

// The full representation of the tag, which is "key:value".
func (t Tag) String() string {
return t.keyValue
return t.key + ":" + t.value
}

type Tags []Tag
Expand Down Expand Up @@ -61,7 +59,10 @@ func (t Tags) Len() int {
}

func (t Tags) Less(i, j int) bool {
return t[i].keyValue < t[j].keyValue
if t[i].key == t[j].key {
return t[i].value < t[j].value
}
return t[i].key < t[j].key
}

func (t Tags) Swap(i, j int) {
Expand Down Expand Up @@ -118,9 +119,8 @@ func newTag(k, v string) Tag {
normalizedKey := normalizeTag(k, validKeyChars)
normalizedValue := normalizeTag(v, validValueChars)
return Tag{
key: normalizedKey,
value: normalizedValue,
keyValue: normalizedKey + ":" + normalizedValue,
key: normalizedKey,
value: normalizedValue,
}
}

Expand Down Expand Up @@ -180,6 +180,17 @@ func init() {
//
// Note that this function does not impose the length restriction described above.
func normalizeTag(in string, validChars [utf8.RuneSelf]bool) string {
foundSpecial := false
for _, r := range in {
if r >= utf8.RuneSelf || !validChars[r] {
foundSpecial = true
break
}
}
if !foundSpecial {
return in
}

var builder strings.Builder
builder.Grow(len(in))
for _, r := range in {
Expand Down
15 changes: 6 additions & 9 deletions metrics/tag_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@ func TestMustTag(t *testing.T) {
actual, err := NewTag("keyWithUpper", "valueWithUpper")
assert.NoError(t, err)
assert.Equal(t, Tag{
key: "keywithupper",
value: "valuewithupper",
keyValue: "keywithupper:valuewithupper",
key: "keywithupper",
value: "valuewithupper",
},
actual)
})
Expand All @@ -27,9 +26,8 @@ func TestMustTag(t *testing.T) {
actual, err := NewTag("a_-./0", "a_-:./0")
assert.NoError(t, err)
assert.Equal(t, Tag{
key: "a_-./0",
value: "a_-:./0",
keyValue: "a_-./0:a_-:./0",
key: "a_-./0",
value: "a_-:./0",
},
actual)
})
Expand All @@ -38,9 +36,8 @@ func TestMustTag(t *testing.T) {
actual, err := NewTag("a(❌)", "a(❌)")
assert.NoError(t, err)
assert.Equal(t, Tag{
key: "a___",
value: "a___",
keyValue: "a___:a___",
key: "a___",
value: "a___",
},
actual)
})
Expand Down