Use labelbuilder in syntax.ParseLabels to remove empty label values #7355
Conversation
This file contains 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
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
MasslessParticle
approved these changes
Oct 6, 2022
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
- querier/queryrange -0.1%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
owen-d
added a commit
that referenced
this pull request
Oct 7, 2022
This PR does two things: * Includes the series fingerprint in TSDB WALs. Doing this ensures we recover fingerprint used in chunk addresses after restart even if loki is upgraded and the label hashing algorithm changes. * Removes much of the extra hashing in the TSDB write path + manager. Occasionally, the fingerprint may be different than the hash of labels. Historically this has happened in a few places: * When the `__name__="logs"` label was injected for non-tsdb indices (not applicable to TSDB) * When building a multitenant tsdb index and adding a synthetic `__loki_tenant__` label * In a previous bug(#7355) when empty label values altered calculated fingerprints. Explicitly storing/retrieving the desired fingerprint throughout helps avoid calculating it in error.
lxwzy
pushed a commit
to lxwzy/loki
that referenced
this pull request
Nov 7, 2022
…rafana#7355) There's a bug in TSDB where empty label values alter the series fingerprints used in chunk addresses, but are stripped out and create different fingerprints in our TSDB based index. I plan to open a PR to Prometheus, but I also prefer normalizing labels here by removing empty label values. According to [Prometheus](https://prometheus.io/docs/concepts/data_model/), > A label with an empty label value is considered equivalent to a label that does not exist. Our bug comes from the Prometheus model `LabelBuilder` stripping out empty label values, [here](https://github.com/grafana/loki/blob/main/vendor/github.com/prometheus/prometheus/model/labels/labels.go#L419-L423). This means that `Labels.NewBuilder({job="foo",bar=""}) => {job="foo"}`. The [labels.Hash()](https://github.com/grafana/loki/blob/main/vendor/github.com/prometheus/prometheus/model/labels/labels.go#L136-L182) function does not skip empty label values, meaning it's easy to end up with different fingerprints for effectively the same series (an empty label value is supposed to be equivalent to a missing label in prometheus, but they generate different label hashes).
lxwzy
pushed a commit
to lxwzy/loki
that referenced
this pull request
Nov 7, 2022
This PR does two things: * Includes the series fingerprint in TSDB WALs. Doing this ensures we recover fingerprint used in chunk addresses after restart even if loki is upgraded and the label hashing algorithm changes. * Removes much of the extra hashing in the TSDB write path + manager. Occasionally, the fingerprint may be different than the hash of labels. Historically this has happened in a few places: * When the `__name__="logs"` label was injected for non-tsdb indices (not applicable to TSDB) * When building a multitenant tsdb index and adding a synthetic `__loki_tenant__` label * In a previous bug(grafana#7355) when empty label values altered calculated fingerprints. Explicitly storing/retrieving the desired fingerprint throughout helps avoid calculating it in error.
changhyuni
pushed a commit
to changhyuni/loki
that referenced
this pull request
Nov 8, 2022
…rafana#7355) There's a bug in TSDB where empty label values alter the series fingerprints used in chunk addresses, but are stripped out and create different fingerprints in our TSDB based index. I plan to open a PR to Prometheus, but I also prefer normalizing labels here by removing empty label values. According to [Prometheus](https://prometheus.io/docs/concepts/data_model/), > A label with an empty label value is considered equivalent to a label that does not exist. Our bug comes from the Prometheus model `LabelBuilder` stripping out empty label values, [here](https://github.com/grafana/loki/blob/main/vendor/github.com/prometheus/prometheus/model/labels/labels.go#L419-L423). This means that `Labels.NewBuilder({job="foo",bar=""}) => {job="foo"}`. The [labels.Hash()](https://github.com/grafana/loki/blob/main/vendor/github.com/prometheus/prometheus/model/labels/labels.go#L136-L182) function does not skip empty label values, meaning it's easy to end up with different fingerprints for effectively the same series (an empty label value is supposed to be equivalent to a missing label in prometheus, but they generate different label hashes).
changhyuni
pushed a commit
to changhyuni/loki
that referenced
this pull request
Nov 8, 2022
This PR does two things: * Includes the series fingerprint in TSDB WALs. Doing this ensures we recover fingerprint used in chunk addresses after restart even if loki is upgraded and the label hashing algorithm changes. * Removes much of the extra hashing in the TSDB write path + manager. Occasionally, the fingerprint may be different than the hash of labels. Historically this has happened in a few places: * When the `__name__="logs"` label was injected for non-tsdb indices (not applicable to TSDB) * When building a multitenant tsdb index and adding a synthetic `__loki_tenant__` label * In a previous bug(grafana#7355) when empty label values altered calculated fingerprints. Explicitly storing/retrieving the desired fingerprint throughout helps avoid calculating it in error.
Abuelodelanada
pushed a commit
to canonical/loki
that referenced
this pull request
Dec 1, 2022
…rafana#7355) There's a bug in TSDB where empty label values alter the series fingerprints used in chunk addresses, but are stripped out and create different fingerprints in our TSDB based index. I plan to open a PR to Prometheus, but I also prefer normalizing labels here by removing empty label values. According to [Prometheus](https://prometheus.io/docs/concepts/data_model/), > A label with an empty label value is considered equivalent to a label that does not exist. Our bug comes from the Prometheus model `LabelBuilder` stripping out empty label values, [here](https://github.com/grafana/loki/blob/main/vendor/github.com/prometheus/prometheus/model/labels/labels.go#L419-L423). This means that `Labels.NewBuilder({job="foo",bar=""}) => {job="foo"}`. The [labels.Hash()](https://github.com/grafana/loki/blob/main/vendor/github.com/prometheus/prometheus/model/labels/labels.go#L136-L182) function does not skip empty label values, meaning it's easy to end up with different fingerprints for effectively the same series (an empty label value is supposed to be equivalent to a missing label in prometheus, but they generate different label hashes).
Abuelodelanada
pushed a commit
to canonical/loki
that referenced
this pull request
Dec 1, 2022
This PR does two things: * Includes the series fingerprint in TSDB WALs. Doing this ensures we recover fingerprint used in chunk addresses after restart even if loki is upgraded and the label hashing algorithm changes. * Removes much of the extra hashing in the TSDB write path + manager. Occasionally, the fingerprint may be different than the hash of labels. Historically this has happened in a few places: * When the `__name__="logs"` label was injected for non-tsdb indices (not applicable to TSDB) * When building a multitenant tsdb index and adding a synthetic `__loki_tenant__` label * In a previous bug(grafana#7355) when empty label values altered calculated fingerprints. Explicitly storing/retrieving the desired fingerprint throughout helps avoid calculating it in error.
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.
There's a bug in TSDB where empty label values alter the series fingerprints used in chunk addresses, but are stripped out and create different fingerprints in our TSDB based index. I plan to open a PR to Prometheus, but I also prefer normalizing labels here by removing empty label values.
According to Prometheus,
Our bug comes from the Prometheus model
LabelBuilderstripping out empty label values, here. This means thatLabels.NewBuilder({job="foo",bar=""}) => {job="foo"}. The labels.Hash() function does not skip empty label values, meaning it's easy to end up with different fingerprints for effectively the same series (an empty label value is supposed to be equivalent to a missing label in prometheus, but they generate different label hashes).