Add and use abstractions over labels.Labels #11717
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use simpler utility function to create Labels objects, making fewer assumptions about the data structure. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
591def7 to
6a87028
Compare
| if l.Name == name { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
does it make sense to assume sorted and do something like if l.Name > name {return false} ?
There was a problem hiding this comment.
I think not: we know conflictingExposedLabels is sorted at the beginning, but after we have renamed one foo to exported_foo it is no longer certain.
74cd473 to
3870b8f
Compare
This code is a bit cleaner. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Without changing the definition of `labels.Labels`, add methods which enable code using it to work without knowledge of the internals. 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>
Re-did the FromStrings test to avoid assumptions about how it works. 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>
Parse metrics using labels.ScratchBuilder, so we reduce assumptions about internals of Labels. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
We don't want to touch the result labels now we create them differently. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Incomplete - needs further changes to `Decoder.Series()`. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
This necessitates a change to the `tsdb.IndexReader` interface: `index.Reader` is used from multiple goroutines concurrently, so we can't have state in it. We do retain a `ScratchBuilder` in `blockBaseSeriesSet` which is iterator-like. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Note in one cases we needed an extra copy of labels in case they change. 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>
Implement decoding via labels.ScratchBuilder, which we retain and re-use to reduce memory allocations. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
We use `labels.Builder` to parse metrics, to avoid depending on the internal implementation. This is not efficient, but the feature is only used in tests. It wasn't efficient previously either - calling `Sort()` after adding each label. `createLabelsForAbsentFunction` also uses a Builder now, and gets an extra `map` to replace the previous `Has()` usage. Signed-off-by: Bryan Boreham <bjboreham@gmail.com> Fix up promql to compile with changes to 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>
`QueueManager.externalLabels` becomes a slice rather than a `Labels` so we can index into it when doing the merge operation. 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 ScratchBuilder 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>
Use `FromStrings` instead of assuming the data structure. And don't sort individual labels, since `labels.Labels` are always sorted. 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 passing in a `ScratchBuilder` and `Labels`, just pass the builder and the caller can extract labels from it. In many cases the caller didn't use the Labels value anyway. Now in `Labels.ScratchBuilder` we need a slightly different API: one to assign what will be the result, instead of overwriting some other `Labels`. This is safer and easier to reason about. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
3870b8f to
10b27df
Compare
|
I changed This is simpler, more foolproof, and faster in a couple of places where the labels value is not used. |
|
@roidelapluie, @bwplotka (since you reviewed related PRs) I am ready to merge this; please let me know if I should wait a little (e.g. until 2.41 is released). |
|
You can merge this, 2.41 is on its branch. |
|
Feel free to go 👍🏽 |
This was referenced Dec 20, 2022
krajorama
added a commit
to krajorama/prometheus
that referenced
this pull request
Jan 8, 2023
Comment was not updated when code changed from labels to builder in prometheus#11717 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
vjsamuel
pushed a commit
to vjsamuel/prometheus
that referenced
this pull request
Jan 9, 2023
Comment was not updated when code changed from labels to builder in prometheus#11717 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is #10887, adding all the abstractions but not actually changing
labels.Labels.This should be possible to merge without breaking too much downstream.
There are one or two breaking changes, like
IndexReader.Seriesgets a newScratchBuilderparameter.