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

feat: limit tags and tag values search #4320

Merged
merged 27 commits into from
Dec 5, 2024

Conversation

javiermolinar
Copy link
Contributor

@javiermolinar javiermolinar commented Nov 13, 2024

What this PR does:

This PR introduces two new optional parameters to enhance the control of Tags and Tag Values lookups:

  • limit:
    Specifies the maximum number of items to retrieve. For the api/v2/tags endpoint, the search stops if any scope reaches this limit.

  • maxStaleValues:
    Restricts the search for tag values. If the number of stale (already known) values meets or exceeds this limit, the search is halted. This is useful when we have a small set of tag values.

Which issue(s) this PR fixes:
Fixes #4288

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@javiermolinar javiermolinar changed the title limit tags and tag values search feat: limit tags and tag values search Nov 13, 2024
@javiermolinar javiermolinar marked this pull request as ready for review November 13, 2024 09:57
@javiermolinar javiermolinar added type/docs Improvements or additions to documentation enhancement New feature or request labels Nov 13, 2024
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

i'd also like to get your thoughts on a different kind of parameter. perhaps it could be added here as well?

what about a parameter that stopped searching early if N strings were added to the combiner and none were new? so the combiner would keep a running count of the number of added strings since a new one was found and stop searching once we crossed threshold.

consider /api/v2/search/tag/resource.cluster/values?q=<some very expensive query>. there are likely very few values for resource.cluster but we still spend a lot of resources to exhaustively evaluate <some very expensive query>. a parameter that allowed the ingester to just stop searching after N found values and nothing new could be a powerful way to dynamically circuit break.

modules/frontend/combiner/search_tags_test.go Outdated Show resolved Hide resolved
pkg/collector/scoped_distinct_string.go Outdated Show resolved Hide resolved
Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

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

Thank you for updating the docs! I have a few minor suggestions to correct missing periods.

@joe-elliott
Copy link
Member

The more I think about this idea the more I want it:

what about a parameter that stopped searching early if N strings were added to the combiner and none were new? so the combiner would keep a running count of the number of added strings since a new one was found and stop searching once we crossed threshold.

The change here and the current max_bytes_per_tag_values_query protect us when a tag value query finds A LOT of matches. We currently have no protection for an expensive tag values query that find a very low number of values.

javiermolinar and others added 9 commits November 26, 2024 11:42
Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>
Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>
Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>
@javiermolinar
Copy link
Contributor Author

I want to add some logging to control how this behaves

docs/sources/tempo/api_docs/_index.md Outdated Show resolved Hide resolved
modules/frontend/tag_handlers.go Outdated Show resolved Hide resolved
pkg/collector/distinct_value_collector.go Outdated Show resolved Hide resolved
pkg/collector/distinct_value_collector_test.go Outdated Show resolved Hide resolved
pkg/collector/distinct_string_collector_test.go Outdated Show resolved Hide resolved
pkg/tempopb/tempo.proto Outdated Show resolved Hide resolved
tempodb/encoding/common/interfaces.go Outdated Show resolved Hide resolved
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

needs a changelog, but otherwise good. great work here.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

🙏

@javiermolinar javiermolinar merged commit 6c9dc98 into grafana:main Dec 5, 2024
17 checks passed
@javiermolinar javiermolinar deleted the search-tags-limit branch December 5, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request type/docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a limit parameter to search tags and tag values endpoints
3 participants