-
Notifications
You must be signed in to change notification settings - Fork 834
Add support for /api/v1/format_query API #6893
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
Add support for /api/v1/format_query API #6893
Conversation
Signed-off-by: Siddarth Gundu <[email protected]>
Signed-off-by: Siddarth Gundu <[email protected]>
Thanks for the contribution. Do we have to require the |
This API is for PromQL pretty-formatting, I'm just wondering if the |
I see, I went through some documentation and based on Prometheus security what I understand is that a standalone prometheus server dosent need any auth because its presumed that untrusted users have access to the Prometheus HTTP endpoint but when cortex acts as a prometheus web server it would still look for Apologies for any confusion I caused |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@siddarth2810 This looks good, but needs an integration test. Can you add it?
I believe auth is handled already. The integration test will prove it. |
Signed-off-by: Siddarth Gundu <[email protected]>
Thanks a lot for the review. I have added the test, when testing for both querier and query Frontend is it required to use separate endpoints or testing is done for both when we just add query frontend ? Let me know if In my other integration test, I got different results with and without using query frontend so I added both cases in this PR Really appreaciate for any inputs, thanks ! Btw when |
@friedrichg @siddarth2810 |
I think I prefer the way it is now. Typically things that do not required the header are things that can be queried by cortex operators, not cortex users. This API path doesn't use any resources, but suppose we impose query limits to this request type, per tenant, or a different format type in the future. We want to do this on a per tenant basis, thus we need the authority header. it's similar to /api/v1/status/buildinfo. |
@siddarth2810 Thanks for implementing the test. There is a lint error still:
|
Signed-off-by: Siddarth Gundu <[email protected]>
My bad. Fixed it ! |
Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thanks
* Add support for /api/v1/format_query API Signed-off-by: Siddarth Gundu <[email protected]> * update changelog Signed-off-by: Siddarth Gundu <[email protected]> * Integration: Add test for format query API Signed-off-by: Siddarth Gundu <[email protected]> * fix lint error Signed-off-by: Siddarth Gundu <[email protected]> --------- Signed-off-by: Siddarth Gundu <[email protected]>
What this PR does:
The PR enables /api/v1/format_query API endpoint for formatting queries on Query Frontend and Querier
Which issue(s) this PR fixes:
Fixes #6670
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]