-
Notifications
You must be signed in to change notification settings - Fork 107
remove tilde from name values when indexing tags #1371
Conversation
// used as a tag value. This is important when we index | ||
// metric names as values of the tag "name" | ||
func SanitizeNameAsTagValue(name string) string { | ||
if !strings.Contains(name, "~") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why all the complexity in this function. Cant you just use strings.ReplaceAll(name, "~", "")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely can, but since this is a function that may get called very often (potentially millions of times in one query) I wanted to keep it as efficient as possible. let me benchmark which version is more efficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the definition of what is valid has just changed, so this function will get updated anyway
graphite-project/carbon#858 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a new version. I benchmarked it and compared it with an alternative version which is simpler by relying on strings
:
func SanitizeNameAsTagValue(name string) (string, error) {
if len(name) == 0 {
return name, fmt.Errorf("Invalid name of length 0")
}
if name[0] != '~' {
return name, nil
}
name = strings.TrimLeft(name, "~")
if len(name) == 0 {
return "", fmt.Errorf("Invalid name, requiring other characters than ~")
}
return name, nil
}
The first one is with the latest commit that I pushed (raintank/schema@f065be1) and the second is with the above simplified version:
replay@vbox:~/go/src/github.com/raintank/schema$ go test -run=none -bench=BenchmarkNameSanitized
goos: linux
goarch: amd64
pkg: github.com/raintank/schema
BenchmarkNameSanitizedAsTagValueWithValidValue-3 300000000 5.02 ns/op
BenchmarkNameSanitizedAsTagValueWithInvalidValue-3 200000000 6.10 ns/op
PASS
ok github.com/raintank/schema 3.867s
replay@vbox:~/go/src/github.com/raintank/schema$ go test -run=none -bench=BenchmarkNameSanitized
goos: linux
goarch: amd64
pkg: github.com/raintank/schema
BenchmarkNameSanitizedAsTagValueWithValidValue-3 300000000 5.09 ns/op
BenchmarkNameSanitizedAsTagValueWithInvalidValue-3 20000000 84.8 ns/op
PASS
ok github.com/raintank/schema 3.810s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I don't think it makes sense for this method to have the potential to return errors, if the returned value is ""
then the caller should just have to deal with it, i'll change that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it again so the method does not return errors and compared the latest version (raintank/schema@1adcebf) with a simpler version relying on strings
. it's quite a difference:
replay@vbox:~/go/src/github.com/raintank/schema$ go test -run=none -bench=BenchmarkNameSanitized
goos: linux
goarch: amd64
pkg: github.com/raintank/schema
BenchmarkNameSanitizedAsTagValueWithValidValue-3 300000000 4.41 ns/op
BenchmarkNameSanitizedAsTagValueWithInvalidValue-3 300000000 5.74 ns/op
PASS
ok github.com/raintank/schema 4.095s
replay@vbox:~/go/src/github.com/raintank/schema$ go test -run=none -bench=BenchmarkNameSanitized
goos: linux
goarch: amd64
pkg: github.com/raintank/schema
BenchmarkNameSanitizedAsTagValueWithValidValue-3 20000000 70.4 ns/op
BenchmarkNameSanitizedAsTagValueWithInvalidValue-3 20000000 82.0 ns/op
PASS
ok github.com/raintank/schema 3.220s
b38778c
to
323e4de
Compare
323e4de
to
d1368da
Compare
This should be ready to review now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nits, lgtm
c2f0a0f
to
23d3f14
Compare
I'll wait for this PR to get merged before this can be merged: raintank/schema@80d1297
This is related to grafana/grafana#15261
And it is the implementation of graphite-project/graphite-web#2458