[k184] fix: align semantics of metric and log query label extraction #11668
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
|
Trivy scan found the following vulnerabilities:
|
trevorwhitney
approved these changes
Jan 11, 2024
|
@trevorwhitney i'm surprised to see the trivy message since you merged #11608. |
|
@dannykopping looks like we need to backport #11608 to k184 |
trevorwhitney
added a commit
that referenced
this pull request
Jan 16, 2024
**What this PR does / why we need it**: A data race introduced in #11587 was caught in the [backport to k184](#11668). This removes the shared state of a single global `noParserHints` in favor of creating an empty `Hint` object for each label builder, since the `Hints` is keeping state of `extracted` and `requiredLabels`.
|
@trevorwhitney it's already in k184 it seems. loki [k184] [14:35:15] $ git cherry-pick 8b48a18
On branch k184
Your branch is up to date with 'upstream/k184'.
You are currently cherry-picking commit 8b48a18d7.
(all conflicts fixed: run "git cherry-pick --continue")
(use "git cherry-pick --skip" to skip this patch)
(use "git cherry-pick --abort" to cancel the cherry-pick operation)
nothing to commit, working tree clean
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:
git commit --allow-empty
Otherwise, please use 'git cherry-pick --skip' |
rhnasc
pushed a commit
to inloco/loki
that referenced
this pull request
Apr 12, 2024
) **What this PR does / why we need it**: A data race introduced in grafana#11587 was caught in the [backport to k184](grafana#11668). This removes the shared state of a single global `noParserHints` in favor of creating an empty `Hint` object for each label builder, since the `Hints` is keeping state of `extracted` and `requiredLabels`.
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.
Backport 9759c13 from #11587
What this PR does / why we need it:
As a result of the query optimization work, we accidentally introduced a discrepancy between the semantics of logs and metrics queries. Metric queries can benefit from a short-circuit during label extraction, where we only need the labels needed for grouping and filtering. Log queries need to always extract all labels, as a user may want to inspect the key=value pairs of all detected fields, not just those filtered on.
However, given a query with nested labels of the same name (ie
{"message": {"message": "foo"}}) this short circuit introduces a problem where the metric query will use the value of the firstmessage(since it stops parsing themessagekey after finding it once), but the log query will use the value of the secondmessage(since it will continue to extract all labels, even those it has already seen). This PR changes the semantics so that both types of queries will only use the first value.The result of the change is a slight improvement in the hot path of label extraction, which I interpret as us having to do a few more operations due to the removal of the
len(requiredLabels) == 0short circuit, but those operations are quick, and thus more are done in the same runtime.Which issue(s) this PR fixes:
Fixes #11647