Hide the implementation details of labels.Labels#10887
Hide the implementation details of labels.Labels#10887bboreham wants to merge 54 commits intoprometheus:mainfrom
Conversation
|
Are you able to test this in real condition to better asses the performance impact? |
|
@roidelapluie maybe let's just run Prombench on this PR? |
|
/prombench main |
|
Benchmark tests are running for 3 days! If this is intended ignore this message otherwise you can cancel it by commenting: |
|
I see very close correspondance between the 'main' and 'PR' lines on the dashboard, which matches what the benchmarks say. |
|
/prombench cancel |
|
Benchmark cancel is in progress. |
|
I pushed an update which removes |
be780fd to
6369b08
Compare
|
Can you please fix linting? |
|
Sure; note #10991 is the interesting one; this is just laying the groundwork. |
|
I wonder if we could merge this and then feature flag this change |
I assume you mean merge #10887 and feature-flag #10991? That is an interesting idea, however the change pervades many parts of Prometheus so I don't think that is realistic. |
|
Oh, I was thinking we could turn labels into an interface |
While conceptually that is a fine idea; in practice we now need 16 bytes for the interface pointer on top of whatever is used to store Labels (e.g. a 24-byte slice header). Also all calls through an interface cause heap escape, which may make some existing operations expensive. So I'm guessing it will be too expensive. |
|
Let's reimplement Prometheus in Rust… 🦆 |
|
Can we move this out of draft? |
|
There have been a few fixes in #10991 which also affect this branch; I should move them here, and particularly the extra tests. I think this change will be disruptive for projects like Thanos and Mimir. I don't think it's a good idea to merge now without some preparation (e.g. trying the port in a fork). |
|
Yet it is an ideal time for Prometheus itself since we are far from a release. I'd really like to see this PR happen asap. Dependencies can decide when they upgrade, I don't think timing is a big issue. |
I have done this; unfortunately in the process I broke it. Will fix soon. Note the |
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Note we avoid calling `Labels.Len()` in `labelProtosToLabels()`. It isn't necessary - `append()` will enlarge the buffer and we're expecting to re-use it many times. Also, we now validate protobuf input before converting to Labels. This way we can detect errors first, and we don't place unnecessary requirements on the Labels structure. Re-do seriesFilter using labels.Builder (albeit N^2). Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Use SimpleBuilder to create labels. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Instead of relying on being able to append to it like a slice. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
This avoids spurious failures in tests. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
In code added since first pass. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
To avoid code breaking when the data structure for labels.Labels changes. In code added since first pass. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Saves memory allocations when you know the right size. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
It was only called in one place, so move the logic there. Reduced surface area of labels.Labels is better for understandability. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
It was only called in one place, so move the logic there. `QueueManager.externalLabels` becomes a slice rather than a `Labels` so we can index into it when doing the merge operation. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
The requirement is met fairly well by `ScratchBuilder`. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
labels.Labels are always sorted. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Much easier to read. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
f7a3c12 to
aeb5c23
Compare
The slice nature of
labels.Labelsis hard-coded in about 1,000 places in the code of Prometheus, which makes it very difficult to experiment with a different data structure.This PR changes the type to a struct so that any attempt to use it as a slice generates a compilation error, and introduces some abstractions to allow these errors to be fixed.
Some of these abstractions (e.g.A newReserve()andAppend()are rather thin and we can probably do better; consider them placeholders if you like.ScratchBuilderwas added, which is likelabels.Builderbut streamlined for the case where we only add unique labels.It's not much of a step forward on its own, but it makes things easier for someone (like me) experimenting with the data structure. Mentioned on prometheus-developers.
All tests pass, and benchmarks go broadly at the same speed.