consul
consul copied to clipboard
Add support for filtering the 'List Services' API
- Create a bexpr filter for performing the filtering
- Change the state store functions to return the raw (not aggregated) list of ServiceNodes.
- Move the aggregate service tags by name logic out of the state store functions into a new function called from the RPC endpoint
- Perform the filtering in the endpoint before aggregation.
TODO:
- [x] update existing tests for the new function signatures
- [x] add new tests for filtering the results of this endpoint
- [x] update the api-docs
This change updates the /v1/catalog/services endpoint to now support the filter parameter. This change is desirable as it brings the API into alignment with other endpoints and more importantly makes it easier for a client to query for only part of the catalog based on more than just node metadata.
This is potentially of particular use to projects such as Prometheus, where scraping the entire catalog and then querying each service individually for their respective metadata is expensive. By making some of Consul's advanced filtering available earlier in the process, this can improve performance of target discovery and reduce resource utilization.
Please note the heavy lifting of updating the RPC/State was done by @dnephin! So, give props to him :)
I've added some of my own comments in-line. Thank you
This PR targets 1.9 because that's what I currently have deployed so that I can test it in my infrastructure. I will rebase accordingly onto main. But I wanted to get some early feedback first.
Addresses #9422
This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.
consul-ui-staging – ./ui
🔍 Inspect: https://vercel.com/hashicorp/consul-ui-staging/ANbiy9jkzVz4PynqYjETocX7gkHh
✅ Preview: Canceled
[Deployment for bf816bb canceled]
Hi @dekimsey, thanks for this PR!
I'd like to request that your PR targets main - we generally do not add features to older releases (unless they are bugfixes or security-related) but even in those cases it is easier to backport from main to the appropriate release/ branches.
Not a problem @kisunji, I updated the PR and rebased it accordingly.
Thanks @dekimsey!
Our team is busy with a some releases this week but will aim to have this reviewed early next week!
@kisunji thank you, no rush! I'm looking forward to the feedback.
Would you be able to rebase from main? Looks like there's been several refactors since this PR was up. Let me know if anything gets complicated.
No worries about taking long to respond! We're thankful for your contributions regardless.
Would you be able to rebase from main?
@kisunji, I see what you mean. I did rebase recently. Somehow git did something silly with catalog_test.go. I'll investigate. Thank you.
I also noticed the changes that now include a few go-cmp cases, I'm updating my tests to follow suit. It resolves the test errors and is far more readable than my initial stab at it which is great!
@kisunji The tests were updated and cleaned up which I think addresses @dnephin's review. Please let me know what you think! Thanks
Thanks @dekimsey! I'm going to sanity check that this won't break any files in Enterprise then will try to get another set of eyes for review.
@Amier3 Is there anything I could/should do to get some eyes on this?
As soon as it's merged, I'd like to pivot to work on a Prometheus PR to use it and take some load off of our scrapers.
Thank you!
Hi all, I just wanted to check-in on the status of this. I've been trying to keep it up-to-date with main as it's moved a few times since I initially opened the MR. Our prometheus instances open a ton of watchers because we don't currently have a good way to prune the target services on the List Services API call, this change would allow us to address that gap. Please let me know what I need to do or change to get this integrated.
Thank you!
Hi @dekimsey,
Sorry for the delay in getting back to you. There are some tests on the enterprise side that need updating so I will work on it and have this merged for our next minor release (1.14)