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

Added support for metadata APIs. #49

Merged
merged 1 commit into from
Dec 28, 2020
Merged

Added support for metadata APIs. #49

merged 1 commit into from
Dec 28, 2020

Conversation

bwplotka
Copy link
Member

Went ahead and implemented all APIs (: Disabled labels behind flag as there is only WIP PRs present right now.

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

Similar to query endpoint, for metadata endpoints `/api/v1/series`, `/api/v1/labels`, `/api/v1/label/<name>/values` the proxy inject the specified label in the `match` selector.

NOTE: At the moment of creation `/api/v1/labels`, `/api/v1/label/<name>/values` does not support `match[]` so they are disabled by default. Use `-enable-label-apis` flag to enable
those (see https://github.com/prometheus/prometheus/issues/6178 for tracking development).
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is a sane default (I was wondering why we don't enable it by default) 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it gives false impression that tenancy is ensured, but literally implementation of label matcher does not exists yet (WIP PRs only)

@s-urbaniak
Copy link
Contributor

LGTM 👍 Leaving final pass to another pair of eyes, cc @brancz @simonpasquier

@brancz
Copy link
Member

brancz commented Oct 26, 2020

How about we wait for the APIs to implement these and then bump the Prometheus version compatibility?

README.md Outdated Show resolved Hide resolved
injectproxy/routes.go Outdated Show resolved Hide resolved
injectproxy/routes.go Show resolved Hide resolved
@bwplotka
Copy link
Member Author

Should be fixed 🤗

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@bwplotka
Copy link
Member Author

bwplotka commented Nov 5, 2020

PTAL @s-urbaniak @simonpasquier

@s-urbaniak
Copy link
Contributor

LGTM 👍 Is there anything we need to do with respect to Frederic's comment in #49 (comment)? I see both upstream issues are still open.

@bwplotka
Copy link
Member Author

bwplotka commented Nov 5, 2020

How about we wait for the APIs to implement these and then bump the Prometheus version compatibility?

I think this will be a bit cruel for users because:

  • Most of the users rarely upgrade Prometheus and there is no need to block them, they can just disable those APIs
  • We can leverage this on sooner

Being forward compatible is quite nice here I would say (:

@brancz
Copy link
Member

brancz commented Nov 6, 2020

I'm ok with the flag, but I wouldn't merge and release this until the upstream API changes have been done, otherwise we may run into the situation that our assumptions now may not match the implementation in prometheus/prometheus.

@yeya24
Copy link
Contributor

yeya24 commented Dec 16, 2020

Can we move forward with this pr if it is ready? Thanos has supported label matchers in thanos-io/thanos@2508a70 so it would be good to have this feature, too

@bwplotka
Copy link
Member Author

I think Prometheus is stil behind. Let's move Prometheus one forward as well (:

@yeya24
Copy link
Contributor

yeya24 commented Dec 22, 2020

@bwplotka The prometheus pr is merged prometheus/prometheus#8301. Can we merge this?

@bwplotka
Copy link
Member Author

I think so. Any objections to merge this? @s-urbaniak @brancz @lilic ?

@brancz
Copy link
Member

brancz commented Dec 28, 2020

lgtm

@bwplotka bwplotka merged commit f37e989 into master Dec 28, 2020
@brancz brancz deleted the series branch December 29, 2020 08:34
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.

5 participants