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

Proposal: Granular Endpoint Configuration #5505

Closed

Conversation

SrushtiSapkale
Copy link
Contributor

Signed-off-by: Srushti Sapkale srushtiisapkale@gmail.com

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

Signed-off-by: Srushti Sapkale <srushtiisapkale@gmail.com>
Signed-off-by: Srushti Sapkale <srushtiisapkale@gmail.com>
@fpetkovski
Copy link
Contributor

Having something like this would be great for using Queriers with Prometheus sidecars. However, I think it can be a big pitfall when federating multiple queriers through a single query endpoint.

A common use case with Thanos to have cluster-local or regional Queriers which are connected globally through one central Querier. The advantage of using Envoy in this case is that it can load-balance gRPC connections, something we currently don't support natively in Thanos. If people used the endpoints config to connect the global to the regional queriers, they will not be able to scale regional queriers due to the persistent nature of gRPC connections.

Because of that, I think this feature should be very clear on when the endpoint config can be useful versus using something like Envoy.

saswatamcode
saswatamcode previously approved these changes Jul 21, 2022
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks for awesome work! 🌟

Some small suggestions! 🙂

@SrushtiSapkale
Copy link
Contributor Author

Yes, thanks! we need to keep in mind that if the endpoint config is implemented we need a solution for scaling the querier. Also as @saswatamcode suggested, I think per store endpt configuration can a better option for distributed querier 👍

Signed-off-by: Srushti Sapkale <srushtiisapkale@gmail.com>
Signed-off-by: Srushti Sapkale <srushtiisapkale@gmail.com>
@SrushtiSapkale
Copy link
Contributor Author

@saswatamcode can you review the changes in the proposal once again?:slightly_smiling_face:

Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Amazing! Some suggestions,

Signed-off-by: Srushti Sapkale <srushtiisapkale@gmail.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Great job! Some suggestions we made during our 1:2


A new CLI option `--endpoints.config`, with no dynamic reloading, which will accept the path to a yaml file is proposed which contains a list as follows :

```yaml
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we want to tacle the @yeya24 request to enable different API

Copy link
Member

Choose a reason for hiding this comment

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

I or @saswatamcode will add more context

mode: ""
```

The YAML file contains set of endpoints (e.g Store API) with optional TLS options. To enable TLS either use this option or deprecated ones --grpc-client-tls* .
Copy link
Member

Choose a reason for hiding this comment

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

Can I specify multiple endpoint.config flags?

Copy link
Member

Choose a reason for hiding this comment

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

SrushtiSapkale and others added 2 commits August 3, 2022 09:38
Signed-off-by: Srushti Sapkale <srushtiisapkale@gmail.com>
@stale
Copy link

stale bot commented Oct 30, 2022

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

@stale stale bot added the stale label Oct 30, 2022
@matej-g
Copy link
Collaborator

matej-g commented Oct 31, 2022

What's the status of this proposal and the implementation @SrushtiSapkale @saswatamcode @bwplotka? Looks like this was marked stale now.

@stale stale bot removed the stale label Oct 31, 2022
@saswatamcode
Copy link
Member

Yup, so the proposal has some bits that need improvement! Will TAL. 🙂

@SrushtiSapkale
Copy link
Contributor Author

This proposal was a part of my lfx mentorship about implementing the endpoint configuration. Need to add some more points, would be happy to take this forward whenever it gets reviewed again 😅

@stale
Copy link

stale bot commented Jan 7, 2023

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

@stale stale bot added the stale label Jan 7, 2023
@fpetkovski fpetkovski mentioned this pull request Jan 23, 2023
2 tasks
@stale
Copy link

stale bot commented May 21, 2023

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

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

Successfully merging this pull request may close these issues.

5 participants