fix metric sort error #1443
fix metric sort error #1443
Conversation
|
Although I agree that sometimes humans debug exposed metrics by manually looking the /metrics endpoint, I believe this endpoint it focused on metrics being scraped by Prometheus. Prometheus doesn't care about the number ordering 🤷 If we introduce a regex sort, I wonder how much are we sacrificing in performance for this change. Do you mind doing some benchmarking and posting the results here? |
linuxgcc
force-pushed
the
main
branch
2 times, most recently
from
b1bb9b5
to
1d8e814
Compare
|
@ArthurSens 1 . touch such file prometheus/internal/metric_sort_test.go
It can be seen from the above comparison that the comparison of 16-core CPU data before and after using regular expressions takes no more than 0.1ms |
|
We can also see a huge increase in memory allocation though 😱 , 8 allocations per operation up to 626 allocations. The allocated memory also increased a lot |
Signed-off-by: heyitao <heyitao@uniontech.com>
There was a problem hiding this comment.
Nice, thanks for this!
I added more points to the discussion #1442 (comment)
@ArthurSens is right, as you said it's "only" 0.1ms but if ops is one comparison.. we do this potentially 10 000 times per scrape making it a second. Number of allocs is one thing (a lot more) but the bump from 200 B to 50Kb is not minor (per comparison!). Maybe I am misinterpreting what "ops" is in your benchmark, maybe it's per full sorting (but for how many series?) In future please paste the benchmark code too, so we can verify.
But performance is one thing, the most important part is:
- if readability hit of NOT having this is big enough to make this change
- if numerical sorting would NOT actually surprise (thus impact readability) of ppl who got used to it
| @@ -46,6 +48,21 @@ func (s MetricSorter) Swap(i, j int) { | |||
| s[i], s[j] = s[j], s[i] | |||
| } | |||
|
|
|||
| func (s MetricSorter) compare(si, sj string) bool { | |||
| // When the number of devices exceeds two digits, a sorting error occurs. | |||
There was a problem hiding this comment.
This is very specific to node exporter, not generic client_golang use, right? client_golang is used in more places than node_exporter, so we have to care about more use cases.
| func (s MetricSorter) compare(si, sj string) bool { | ||
| // When the number of devices exceeds two digits, a sorting error occurs. | ||
| // dealing with sorting problems, especially CPU sorting errors | ||
| re := regexp.MustCompile(`\d+$`) |
There was a problem hiding this comment.
Hm I think we can't use regex in such a hot path, we might need to be more smarter here if we ever do this ( let's talk on the issue) - or do it optionally.
#1442