-
Notifications
You must be signed in to change notification settings - Fork 179
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
prometheus: improve flatten key performance. #138
prometheus: improve flatten key performance. #138
Conversation
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, I'm happy to merge this as is if you don't think trying replacer
is worthwhile but it seems like a quick thing to try and a simpler solution in general?
prometheus/prometheus.go
Outdated
key = forbiddenChars.ReplaceAllString(key, "_") | ||
|
||
// Only run the regex replace if the key contains forbidden characters to | ||
// void overhead on well-formed keys. |
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.
// void overhead on well-formed keys. | |
// avoid overhead on well-formed keys. |
I wonder if it's worth a link to this PR to show this wasn't a premature optimization and actually helps?
prometheus/prometheus.go
Outdated
// Only run the regex replace if the key contains forbidden characters to | ||
// void overhead on well-formed keys. | ||
if strings.ContainsAny(key, " .=-/") { | ||
key = forbiddenChars.ReplaceAllString(key, "_") |
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 don't know if it's worth the time to try it, but it should only be a few minutes...
I wonder if it would be better to just remove the regex entirely and use strings.Replacer
instead. My guess is that would be just as efficient as the contains
call you added above even in the case where nothing is matched and more efficient even when there are replacements to be made as well as "the right tool for the job" from the stdlib?
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.
Hey @banks and thanks for the suggestion and the snippet of knowledge around the replacer function. I have updated the change to replace the regex with a replacer which performs on-par/better than skipping the regex with well-formed keys, and much better with keys that contain forbidden characters. The PR comment has been updated to show these new benchmarks.
2289c4e
to
efa104e
Compare
The flattenKey function is called whenever a gauge, counter, or sample is set when using the Prometheus sink. The change most notably uses a replacer rather than a regex. The hash formatting also now uses + for concatenation rather than fmt.Sprintf for an addition perf improvement.
efa104e
to
2a21e9d
Compare
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.
Nice LGTM!
The flattenKey function is called whenever a gauge, counter, or sample is set when using the Prometheus sink. The change most notably uses a replacer rather than a regex. The hash formatting also now uses + for concatenation rather than fmt.Sprintf for an addition perf improvement.
The Nomad team recently looked into some Nomad metrics which indicated a fair amount of CPU time spent within the Prometheus
flattenKey
function. While this behaviour was likely exacerbated by other cluster stresses, a small investigation found a couple of potential improvements to the function to improve its performance.Benchmark Code
Benchmark Results:
$ go test -bench=. -benchmem goos: darwin goarch: amd64 pkg: github.com/jrasell/dev-mess/go/benchmark/go-metrics cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz BenchmarkPrometheus_flattenKey/3_parts_1_label_current-16 1499564 893.0 ns/op 250 B/op 8 allocs/op BenchmarkPrometheus_flattenKey/3_parts_1_label_skip-regex-16 7859928 129.4 ns/op 104 B/op 2 allocs/op BenchmarkPrometheus_flattenKey/3_parts_1_label_replacer-16 10091814 127.3 ns/op 104 B/op 2 allocs/op BenchmarkPrometheus_flattenKey/4_parts_5_labels_current-16 689094 1736 ns/op 1017 B/op 24 allocs/op BenchmarkPrometheus_flattenKey/4_parts_5_labels_skip-regex-16 2907072 408.4 ns/op 616 B/op 6 allocs/op BenchmarkPrometheus_flattenKey/4_parts_5_labels_replacer-16 3008982 399.4 ns/op 616 B/op 6 allocs/op BenchmarkPrometheus_flattenKey/forbidden_chars_current-16 503493 2264 ns/op 1115 B/op 26 allocs/op BenchmarkPrometheus_flattenKey/forbidden_chars_skip-regex-16 771224 1590 ns/op 784 B/op 11 allocs/op BenchmarkPrometheus_flattenKey/forbidden_chars_replacer-16 2389686 497.4 ns/op 736 B/op 8 allocs/op PASS ok github.com/jrasell/dev-mess/go/benchmark/go-metrics 14.985s