consul icon indicating copy to clipboard operation
consul copied to clipboard

Add support for filtering the 'List Services' API

Open dekimsey opened this issue 3 years ago • 11 comments

  1. Create a bexpr filter for performing the filtering
  2. Change the state store functions to return the raw (not aggregated) list of ServiceNodes.
  3. Move the aggregate service tags by name logic out of the state store functions into a new function called from the RPC endpoint
  4. 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

dekimsey avatar Dec 05 '21 20:12 dekimsey

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]

vercel[bot] avatar Dec 05 '21 20:12 vercel[bot]

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.

kisunji avatar Dec 14 '21 20:12 kisunji

Not a problem @kisunji, I updated the PR and rebased it accordingly.

dekimsey avatar Dec 14 '21 21:12 dekimsey

Thanks @dekimsey!

Our team is busy with a some releases this week but will aim to have this reviewed early next week!

kisunji avatar Dec 15 '21 18:12 kisunji

@kisunji thank you, no rush! I'm looking forward to the feedback.

dekimsey avatar Dec 15 '21 22:12 dekimsey

CLA assistant check
All committers have signed the CLA.

hashicorp-cla avatar Mar 12 '22 17:03 hashicorp-cla

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.

kisunji avatar Mar 21 '22 14:03 kisunji

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!

dekimsey avatar Mar 21 '22 14:03 dekimsey

@kisunji The tests were updated and cleaned up which I think addresses @dnephin's review. Please let me know what you think! Thanks

dekimsey avatar May 13 '22 15:05 dekimsey

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.

kisunji avatar May 17 '22 14:05 kisunji

@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!

dekimsey avatar Jul 19 '22 14:07 dekimsey

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!

dekimsey avatar Aug 10 '22 22:08 dekimsey

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)

kisunji avatar Aug 25 '22 20:08 kisunji