Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose optional label matcher for label values handler #8824

Merged
merged 18 commits into from Mar 30, 2023

Conversation

periklis
Copy link
Collaborator

@periklis periklis commented Mar 16, 2023

What this PR does / why we need it:
This is a follow-up to expose the optional matchers on the labels values API to the HTTP handler introduced by #4933

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Currently this works on the querier http API directly. However I still fail to pass the query from the frontend tripperware down the stack when request encoding happens. @jeschkies any advise welcome.

In addition I err to rename the parameter from query to match. However I hesitated to do so as per the LokiLabelNamesRequest implementing the queryrangebase/Request interface that in turns includes a Query() method.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@periklis periklis requested a review from jeschkies March 16, 2023 21:35
@periklis periklis requested a review from a team as a code owner March 16, 2023 21:35
@periklis periklis self-assigned this Mar 16, 2023
pkg/querier/http.go Outdated Show resolved Hide resolved
pkg/querier/queryrange/codec.go Outdated Show resolved Hide resolved
pkg/querier/queryrange/roundtrip.go Outdated Show resolved Hide resolved
@periklis periklis added backport release-2.7.x add to a PR to backport it into release 2.7.x backport release-2.8.x labels Mar 17, 2023
@grafanabot
Copy link
Collaborator

Hello @periklis!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@grafanabot
Copy link
Collaborator

Hello @periklis!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Mar 17, 2023
@periklis periklis added the type/bug Somehing is not working as expected label Mar 17, 2023
@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 18, 2023
@periklis
Copy link
Collaborator Author

@jeschkies @DylanGuedes Considering on how long we have #4933 in our code base remaining unexposed it may still be buggy returning duplicate label values when using a regex label matcher.

Assuming you use the k8s pod names as labels which are per se always unique, I want to filter the values for a set of namespaces I pass to labels values API via a regex matcher, e.g. 4 namespaces each 5 pods:

/loki/api/v1/label/kubernetes_pod_name/values?query={kubernetes_namespace_name=~"ns-a|ns-b|ns-c|ns-d"}

The resulting list includes duplicates (in total 25 pod names, of which 5 are duplicates):

[
  "logger-57f5c5f65d-2wvh6",
  "logger-57f5c5f65d-4tm75",
  **"logger-57f5c5f65d-64pnf",**
  ** "logger-57f5c5f65d-64pnf",**
  "logger-57f5c5f65d-6kr2n",
  "logger-57f5c5f65d-7mpdb",
  "logger-57f5c5f65d-8lqng",
  "logger-57f5c5f65d-9njgp",
  "logger-57f5c5f65d-c5xv7",
  **"logger-57f5c5f65d-cbzpg",**
  **"logger-57f5c5f65d-cbzpg",**
  "logger-57f5c5f65d-dmw8k",
  "logger-57f5c5f65d-g658h",
  "logger-57f5c5f65d-kcvpm",
  "logger-57f5c5f65d-mfbmf",
  **"logger-57f5c5f65d-p2n5x",**
  **"logger-57f5c5f65d-p2n5x",**
  "logger-57f5c5f65d-q7h6d",
  **"logger-57f5c5f65d-r72wk",**
  **"logger-57f5c5f65d-r72wk",**
  "logger-57f5c5f65d-t8nlk",
  "logger-57f5c5f65d-t9tl9",
  **"logger-57f5c5f65d-ttkkh",**
  **"logger-57f5c5f65d-ttkkh",**
  "logger-57f5c5f65d-wj6lj"
]

I have tried to debug this adding some matchers tests in f8ee569 for the series_index_store. However it looks the stores work fine for every schema and config so far. Any clue what could cause such a behavior? (maybe missing dedup work on the query frontend?)

require.Equal(t, ctx, got.Context())
require.Equal(t, "/loki/api/v1/labels/__name__/values", got.URL.Path)
// require.Equal(t, fmt.Sprintf("%d", start.UnixNano()), got.URL.Query().Get("start"))
// require.Equal(t, fmt.Sprintf("%d", end.UnixNano()), got.URL.Query().Get("end"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with commented lines? Is this WIP?

@JStickler
Copy link
Contributor

[docs team] So, looking at both this PR and #4933, and there's just a single line of new documentation? I feel like this needs more doc.

@periklis periklis mentioned this pull request Mar 29, 2023
Copy link
Contributor

@jeschkies jeschkies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a few tiny open questions. Otherwise it looks good.

As for your error. Do you get duplicates without passing label marchers?

pkg/querier/queryrange/roundtrip.go Outdated Show resolved Hide resolved
pkg/querier/queryrange/stats.go Outdated Show resolved Hide resolved
Copy link
Contributor

@DylanGuedes DylanGuedes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just a few nits

pkg/logproto/logproto.proto Outdated Show resolved Hide resolved
pkg/querier/queryrange/codec_test.go Outdated Show resolved Hide resolved
docs/sources/api/_index.md Outdated Show resolved Hide resolved
@periklis periklis merged commit 1bcf683 into grafana:main Mar 30, 2023
4 checks passed
@grafanabot
Copy link
Collaborator

The backport to release-2.7.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-8824-to-release-2.7.x origin/release-2.7.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 1bcf683513b5e11fb1dcc5625761b9760cfbd958
# Push it to GitHub
git push --set-upstream origin backport-8824-to-release-2.7.x
git switch main
# Remove the local backport branch
git branch -D backport-8824-to-release-2.7.x

Then, create a pull request where the base branch is release-2.7.x and the compare/head branch is backport-8824-to-release-2.7.x.

@grafanabot
Copy link
Collaborator

The backport to release-2.8.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-8824-to-release-2.8.x origin/release-2.8.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 1bcf683513b5e11fb1dcc5625761b9760cfbd958
# Push it to GitHub
git push --set-upstream origin backport-8824-to-release-2.8.x
git switch main
# Remove the local backport branch
git branch -D backport-8824-to-release-2.8.x

Then, create a pull request where the base branch is release-2.8.x and the compare/head branch is backport-8824-to-release-2.8.x.

periklis added a commit to periklis/loki that referenced this pull request Mar 30, 2023
jeschkies pushed a commit that referenced this pull request Mar 30, 2023
#8824) (#8960)

**What this PR does / why we need it**:
This is a backport of #8824 into release-2.8

**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:

(cherry picked from commit 1bcf683)

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [x] Documentation added
- [x] Tests updated
- [x] `CHANGELOG.md` updated
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`
periklis added a commit to periklis/loki that referenced this pull request Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-2.7.x add to a PR to backport it into release 2.7.x backport release-2.8.x backport-failed size/L type/bug Somehing is not working as expected type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants