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

[Tag value search] Support q query param in /api/v2/search/<tag.name>/values #2253

Merged
merged 44 commits into from
Apr 24, 2023

Conversation

mapno
Copy link
Member

@mapno mapno commented Mar 23, 2023

What this PR does:

Add support for passing a traceql query to tag values search, via query param q. The query can be incomplete—eg. { .cluster = "foo" && .namespace = }. In such cases, Tempo will extract the value matchers, ie. foo = "bar", and build a query that can be parse by the storage layer.

This is done with a regex that captures the valid matchers. Below is a benchmark of this function for some queries. Overall, the cost of extracting the matchers is very low, and shouldn't affect latency.

go test -bench=BenchmarkExtractMatchers -run=^$ ./pkg/traceql
goos: darwin
goarch: amd64
pkg: github.com/grafana/tempo/pkg/traceql
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
BenchmarkExtractMatchers/{.service_name_=_"foo"}-8                                                                                                 	  786459	      1482 ns/op
BenchmarkExtractMatchers/{.service_name_=_"foo"_&&_.http.status_code_=_200}-8                                                                      	  405924	      2781 ns/op
BenchmarkExtractMatchers/{.service_name_=_"foo"_&&_.http.status_code_=_200_&&_.http.method_=_"GET"}-8                                              	  301669	      3908 ns/op
BenchmarkExtractMatchers/{.service_name_=_"foo"_&&_.http.status_code_=_200_&&_.http.method_=_"GET"_&&_.http.url_=_"/foo"}-8                        	  450922	      2730 ns/op
BenchmarkExtractMatchers/{.service_name_=_"foo"_&&_.cluster_=_}-8                                                                                  	  453796	      2488 ns/op
BenchmarkExtractMatchers/{.service_name_=_"foo"_&&_.http.status_code_=_200_&&_.cluster_=_}-8                                                       	  297260	      3895 ns/op
BenchmarkExtractMatchers/{.service_name_=_"foo"_&&_.http.status_code_=_200_&&_.http.method_=_"GET"_&&_.cluster_=_}-8                               	  232723	      5048 ns/op
BenchmarkExtractMatchers/{.service_name_=_"foo"_&&_.http.status_code_=_200_&&_.http.method_=_"GET"_&&_.http.url_=_"/foo"_&&_.cluster_=_}-8         	  454668	      2646 ns/op
PASS
ok  	github.com/grafana/tempo/pkg/traceql	9.972s

Which issue(s) this PR fixes:
Contributes to #1868

Checklist

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

@mapno mapno changed the title [Tag value search] First approach using traceql engine [Tag value search] Support q query param in /api/v2/search/<tag.name>/values Mar 28, 2023
@knylander-grafana
Copy link
Contributor

Do we need to add docs for this?

@mapno
Copy link
Member Author

mapno commented Mar 29, 2023

Do we need to add docs for this?

Yes. I will update the API docs shortly 👍

@mapno mapno marked this pull request as ready for review March 29, 2023 10:28
pkg/traceql/engine.go Outdated Show resolved Hide resolved
// TODO: Merge into a single regular expression

// Regex to extract selectors from a query string
// This regular expression matches a string that contains a single selector and no OR `||` conditions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change selector to spanset filter which matches the traceql definitions, and add a few examples of queries that match or don't match.

}

collectAttributeValue := func(s Span) bool {
switch wantAttr.Scope {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since wantAttr.Scope doesn't change, we should move this switch logic out of the loop, it can be done once before the Filter function.

for _, ss := range evalSS {
for _, s := range ss.Spans {
if collectAttributeValue(s) {
break evalLoop // Exit early if we have enough data
Copy link
Contributor

Choose a reason for hiding this comment

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

We can return io.EOF to completely stop the Fetch.

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 - Left a few comments, which we discussed offline.

@mapno mapno enabled auto-merge (squash) April 21, 2023 15:34
@mapno mapno disabled auto-merge April 22, 2023 15:19
@mapno mapno merged commit d30364c into grafana:main Apr 24, 2023
@mapno mapno deleted the tag-search-query-param-first-pass branch April 24, 2023 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants