Open cristiangreco opened 1 month ago
- Does this approach leave us open to duplicate labels that the map based implementation was hiding and will prometheus error/panic for duplicate labels?
Yes this edge case is very possible, e.g. if cloudwatch returns any duplicate dimension. I've pushed a change that validates the correct length of labels within EnsureLabelConsistencyAndRemoveDuplicates
.
This refactoring stems from an attempt to optimise memory usage in BuildMetrics and createPrometheusLabels, where labels are copied across various maps. The new PrometheusMetric uses slices to store label pairs and is implemented to guarantee that labels are always sorted by key. The rationale is that slices might be more memory efficient than maps for large preallocation sizes. Moreover, the fact that label keys are promptly available (no need to iterate over the map) comes handy in a bunch of places where we save additional allocations. Lastly, while we spend cycles to do explicit sorting in yace now, it should save us some comparisons when prometheus sorts labels internally.
The refactoring also comes with a reimplementation of signature for labels, since the prometheus models only work with maps.
I've added a bunch of benchmarks of specific methods. They show that sometimes the change is noticeable, sometimes it's not (but the overall impact is hard to judge in synthetic benchs due to the variety of input one can get at runtime fromcoming from large aws responses).
Benchmark_EnsureLabelConsistencyAndRemoveDuplicates:
Benchmark_createPrometheusLabels:
Benchmark_BuildMetrics:
Benchmark_NewPrometheusCollector: