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

Search: parse tags from a tags query parameter #1055

Merged
merged 14 commits into from
Oct 20, 2021

Conversation

yvrhdn
Copy link
Member

@yvrhdn yvrhdn commented Oct 19, 2021

What this PR does:
Experiment to try out parsing search request tags from a tags query parameter.

Instead of specifying every tag as it's own query parameter, we put them all in the tags query parameter, separated by a space (logfmt style).
To remain compatible with the current Grafana search implementation we still parse top-level tag query parameters. As Grafana updates this can be removed.

The actual parsing of tags is handled by go-logfmt.

In a next PR I plan to move this code into pkg/api and reuse code with ParseBackendSearch (if it makes sense)

Which issue(s) this PR fixes:
Experiment related to #1038

Checklist

  • Tests updated
  • Documentation added --> docs: update search API #1062
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@yvrhdn yvrhdn force-pushed the search-api-isolate-tags branch from 7e1599e to 2f579ef Compare October 19, 2021 10:03
@yvrhdn
Copy link
Member Author

yvrhdn commented Oct 19, 2021

You can send a test request using curl:

curl -Gs http://localhost:3200/api/search --data-urlencode 'tags=service.name=tempo http.url=api/search' --data-urlencode minDuration=100ms

This will send the following request:

/api/search?tags=service.name%3Dtempo%20http.url%3Dapi%2Fsearch&minDuration=100ms

@yvrhdn yvrhdn marked this pull request as ready for review October 20, 2021 11:15
@yvrhdn yvrhdn mentioned this pull request Oct 20, 2021
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

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

LGTM. Agree with using logfmt (to start at least). Addresses all of my concerns in #1038 (comment)

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.

3 participants