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

gRPC: Isolate Info to separate gRPC API so we can reuse discovery for different RPCs: Store, Rules and Targets #2600

Open
bwplotka opened this issue May 13, 2020 · 61 comments

Comments

@bwplotka
Copy link
Member

Something decided on our public meeting: https://docs.google.com/document/d/165L9DVKHW0sS2PfKYsUCDikwq5XngKkCqTOVjzC6qQE/edit#heading=h.7dcto2wgvh4q

Something to do in later step. This will avoid us to have all those weird flags like separate SD, separate store etc.
We can disable certain APIs if needed on API level (e.g ruler can disable RulesAPI if needed) (:

cc @s-urbaniak

@povilasv
Copy link
Member

I think I would vote for new flag and StoreInfo endpoint returning types supported, like previous proposal mentioned.

Plus on the sidecar you could do --rule-api=disabled , --store-api=disabled or smth similiar.

Thoughts? Maybe we can write a mini proposal for this?

I'm also thinking that potentially we could do TargetsAPI which would show leaf node Targets scraped (I mean Prometheus /targets endpoint) and dedup them? Thoughts?

@bwplotka
Copy link
Member Author

I think I would vote for new flag

New flag? what flag?

and StoreInfo endpoint returning types supported, like previous proposal mentioned.

Hm, yea we wanted to different info per type so its kind of equivalent. So we can detect things either by grpc reflection or info... what would be better? 🤔

@bwplotka
Copy link
Member Author

Also for target API there is issue already: #1375

@bwplotka
Copy link
Member Author

Ok we discussed offline that it would be amazing to actually split discovery as separate gRPC API.

I think we can do it after Rules API is landed, but overall idea is that Info endpoint is shared and Querier will only wait for --endpoint flag or something which will then ask for info API and THEN discover what APIs are there (: This will ensure things like #2407 (comment) and per store TLS will be much easier to implement.

cc @brancz @s-urbaniak @kakkoyun @squat @povilasv @GiedriusS

@povilasv
Copy link
Member

👍 I like --endpoint

@bwplotka bwplotka changed the title RulesAPI: Add gRPC reflection for auto type discovery gRPC: Isolate Info to separate gRPC API so we can reuse discovery for different RPCs: Store, Rules and Targets May 20, 2020
@bwplotka
Copy link
Member Author

Changed title.

@stale
Copy link

stale bot commented Jun 19, 2020

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Jun 19, 2020
@stale
Copy link

stale bot commented Jun 26, 2020

Closing for now as promised, let us know if you need this to be reopened! 🤗

@stale stale bot closed this as completed Jun 26, 2020
@GiedriusS
Copy link
Member

Still valid.

@stale
Copy link

stale bot commented Jul 26, 2020

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Jul 26, 2020
@stale
Copy link

stale bot commented Aug 2, 2020

Closing for now as promised, let us know if you need this to be reopened! 🤗

@stale stale bot closed this as completed Aug 2, 2020
@GiedriusS GiedriusS reopened this Aug 2, 2020
@stale stale bot removed the stale label Aug 2, 2020
@bwplotka
Copy link
Member Author

We need that! (:

@stale
Copy link

stale bot commented Sep 9, 2020

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Sep 9, 2020
@stale
Copy link

stale bot commented Sep 23, 2020

Closing for now as promised, let us know if you need this to be reopened! 🤗

@stale stale bot closed this as completed Sep 23, 2020
@yeya24 yeya24 reopened this Sep 24, 2020
@stale stale bot added the stale label Aug 3, 2021
@GiedriusS GiedriusS removed the stale label Aug 3, 2021
@stale
Copy link

stale bot commented Oct 11, 2021

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Oct 11, 2021
@stale
Copy link

stale bot commented Oct 30, 2021

Closing for now as promised, let us know if you need this to be reopened! 🤗

@stale stale bot closed this as completed Oct 30, 2021
@GiedriusS GiedriusS reopened this Oct 30, 2021
@stale stale bot removed the stale label Oct 30, 2021
@matej-g
Copy link
Collaborator

matej-g commented Nov 12, 2021

Just to keep tabs on this, since now #4282 has been merged, what's still missing? Some points:

cc @hitanshu-mehta - as I said if you'd like a hand with these, happy to help

@hitanshu-mehta
Copy link
Contributor

There was also a request to add a feature to disable certain APIs.

cc @hitanshu-mehta - as I said if you'd like a hand with these, happy to help

That would be great :) Is it okay if we divide tasks?

@matej-g
Copy link
Collaborator

matej-g commented Nov 13, 2021

@hitanshu-mehta I updated the list, thanks 👍 Sure, let's divide! Feel free to take what you prefer first.

@hitanshu-mehta
Copy link
Contributor

@matej-g I can start working on adding new endpoint-strict flag.

@GiedriusS
Copy link
Member

Just to keep tabs on this, since now #4282 has been merged, what's still missing? Some points:

cc @hitanshu-mehta - as I said if you'd like a hand with these, happy to help

Only the last two points are left, I think 💪

@matej-g
Copy link
Collaborator

matej-g commented Nov 26, 2021

So I believe for the API disabling we have overlap with #4785 which should take care of it (one of TODOs - https://github.com/thanos-io/thanos/pull/4785/files#diff-5d0883e9f6db360a29dd686b1ba6dbf152d0ab9549a0506aeffdf532f5ae0591)

@Namanl2001 any updates on the PR progress? Any way we can support you? 🙂

@Namanl2001
Copy link
Contributor

Namanl2001 commented Nov 27, 2021

@Namanl2001, any updates on the PR progress? Any way we can support you? 🙂

Many changes are proposed in this commit, so I definitely need support to proceed further.

In PR #4785, we are trying to have a single endpoint set in cmd/thanos/query.go. For context: #4389 (comment)

Currently, I'm blocked from implementing this function . Help needed.
Thanks

@stale
Copy link

stale bot commented Mar 2, 2022

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Mar 2, 2022
@GiedriusS GiedriusS removed the stale label Mar 16, 2022
@stale
Copy link

stale bot commented Jun 12, 2022

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Jun 12, 2022
@kakkoyun kakkoyun removed the stale label Jun 13, 2022
@stale
Copy link

stale bot commented Aug 13, 2022

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Aug 13, 2022
@GiedriusS
Copy link
Member

Endpoint config is a WIP by our mentee #5585 💪

@stale stale bot removed the stale label Aug 16, 2022
@stale
Copy link

stale bot commented Nov 13, 2022

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests