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

Allow Engines method to take additional parameters #242

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Apr 18, 2023

This allows RemoteEndpoints implementation to use different engines based on the query itself.
For example, in my usecase I will select different engines based on query, start, end and lookback delta value.

Signed-off-by: Ben Ye <benye@amazon.com>
@yeya24 yeya24 requested a review from fpetkovski April 18, 2023 04:35
Signed-off-by: Ben Ye <benye@amazon.com>
@yeya24
Copy link
Contributor Author

yeya24 commented Apr 18, 2023

Upgraded Prometheus version to latest main and also includes prometheus/prometheus@fd3630b.

The point of having ctx is to allow downstream optimizer to be aware of the user. We can extract user ID from the request ctx.

Signed-off-by: Ben Ye <benye@amazon.com>
@yeya24
Copy link
Contributor Author

yeya24 commented Apr 21, 2023

Kindly ping for review.

@fpetkovski
Copy link
Collaborator

Is this needed because we cannot add new logical optimizers from outside the engine?

@yeya24
Copy link
Contributor Author

yeya24 commented Apr 22, 2023

No, even if we can define own optimizer, the engines interface is still unaware of the query, time and other metadata like userid so it cannot know which remote engines it should pick.

@fpetkovski
Copy link
Collaborator

But could you then prune engines in the optimizer itself? The same parameters are present in the logical plan optimizer interface.

@yeya24
Copy link
Contributor Author

yeya24 commented Apr 22, 2023

Got it, I think it works but the RemoteEngine interface will be bypassed for me. However, filtering by external labels doesn't work for me so I need to write my own logic so this way I am just using the Optimizer, not the RemoteEngine interface. In some sense, this is a workaround.

But as you said, since we cannot define external optimizer, it is still not doable.

@fpetkovski
Copy link
Collaborator

fpetkovski commented Apr 22, 2023

I think it's fine to expose the parser publicly because optimizers are an extension point for the engine. Feel free to raise a PR exposing what you need if that is blocking you right now. Ideally we could use the PromQL structs directly, but we can try to get there incrementally.

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.

None yet

2 participants