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

match external labels in exemplars API #4123

Merged
merged 4 commits into from
Apr 30, 2021

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Apr 29, 2021

Signed-off-by: yeya24 yb532204897@gmail.com

Fixes #4116

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Add logic of matching external labels in the exemplars API proxy.

The matching part is a little bit complex because the input is a query, which might contain multiple vector selectors.
We need to match each vector selector separately and construct the final query after all matches are processed.

Verification

Tested locally and one unit test.
We don't have an E2E test for exemplars now so no E2E test added.

Signed-off-by: yeya24 <yb532204897@gmail.com>
Signed-off-by: yeya24 <yb532204897@gmail.com>
@yeya24 yeya24 requested review from squat, kakkoyun and bwplotka and removed request for squat, kakkoyun and bwplotka April 29, 2021 15:17
@kakkoyun kakkoyun requested a review from bwplotka April 29, 2021 15:35
@yeya24 yeya24 force-pushed the fix-exemplar-external-labels branch from 91220c4 to 835c740 Compare April 29, 2021 15:38
pkg/exemplars/proxy.go Outdated Show resolved Hide resolved
@yeya24 yeya24 force-pushed the fix-exemplar-external-labels branch from 835c740 to 457152f Compare April 29, 2021 15:40
@kakkoyun
Copy link
Member

Thanks, @yeya24, this looks amazing. I need to muster some brainpower to review this.

Signed-off-by: yeya24 <yb532204897@gmail.com>
@yeya24 yeya24 force-pushed the fix-exemplar-external-labels branch from 457152f to 55530c4 Compare April 29, 2021 15:41
@yeya24
Copy link
Contributor Author

yeya24 commented Apr 29, 2021

@kakkoyun Thanks for the quick response. Yes it is a little bit complex and hard to review it directly from the code. It would be good to deploy it and test the external labels matching feature.

@tcolgate It would be good if you can help test this pr as well.

Signed-off-by: yeya24 <yb532204897@gmail.com>
@tcolgate
Copy link
Contributor

I've tested this change locally and we are now seeing exemplars on our staging environment (querying via both layers of thanos querier). LGTM

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM

es := &exemplarsStream{
client: exemplarsClient,
request: req,

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

nit

@kakkoyun kakkoyun merged commit 639bccf into thanos-io:release-0.20 Apr 30, 2021
kakkoyun pushed a commit to kakkoyun/thanos that referenced this pull request Apr 30, 2021
* match external labels in exemplars API

Signed-off-by: yeya24 <yb532204897@gmail.com>

* update changelog

Signed-off-by: yeya24 <yb532204897@gmail.com>

* fix lint

Signed-off-by: yeya24 <yb532204897@gmail.com>

* fix flaky tests

Signed-off-by: yeya24 <yb532204897@gmail.com>

* Fix changelog

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
kakkoyun added a commit that referenced this pull request Apr 30, 2021
* match external labels in exemplars API

Signed-off-by: yeya24 <yb532204897@gmail.com>

* update changelog

Signed-off-by: yeya24 <yb532204897@gmail.com>

* fix lint

Signed-off-by: yeya24 <yb532204897@gmail.com>

* fix flaky tests

Signed-off-by: yeya24 <yb532204897@gmail.com>

* Fix changelog

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

Co-authored-by: Ben Ye <yb532204897@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants