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

[TraceQL] Implement structural operators #2625

Merged
merged 20 commits into from
Jul 12, 2023

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented Jul 7, 2023

What this PR does:
Implements descendant (>>), child (>), and sibling (~) structural operators in vParquet2. The TraceQL engine now passes new intrinsics to storage so it can load the minimum columns (each operator uses a different set of columns). The Span interface was updated with generic methods DescendantOf(span) that the engine calls, so it is agnostic to how this data is represented in the backend. I.e. we are not exposing the raw left/right/parent integers to the engine. There are pros and cons to this approach, it is more flexible and as we implement TraceQL on other formats (live traces?), I think it will be better. The drawback is it means a very naive algorithm in the engine to detect the relationships O(n * m). If the engine had the integers it could do fancy things like put them in a map or sorted slice. I think for most cases n and m will be small.

Note: My formatter is now gofumpt, so there is a little bit of noise from the fumpting.

Which issue(s) this PR fixes:
Fixes ?? <-- If we have an issue to track structural operators I couldn't find it.

Checklist

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

pkg/traceql/ast_execute.go Outdated Show resolved Hide resolved
pkg/traceql/enum_attributes.go Show resolved Hide resolved
tempodb/encoding/vparquet2/block_traceql.go Show resolved Hide resolved
tempodb/encoding/vparquet2/block_traceql.go Outdated Show resolved Hide resolved
pkg/traceql/storage.go 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.

looks great! obvious asks: changelog and docs for the new traceql features

@mdisibio mdisibio merged commit ac63a44 into grafana:main Jul 12, 2023
@mapno mapno added type/bug Something isn't working backport r103 and removed type/bug Something isn't working labels Jul 14, 2023
@github-actions
Copy link
Contributor

The backport to r103 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-2625-to-r103 origin/r103
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x ac63a44eb82520d01c42bab70cfab64e4328c3fb
# When the conflicts are resolved, stage and commit the changes
git add . && git cherry-pick --continue

If you have the GitHub CLI installed:

# Create the PR body template
gh pr view 2625 --json body --template 'Backport ac63a44eb82520d01c42bab70cfab64e4328c3fb from #2625{{ "\n\n---\n\n" }}{{ index . "body" }}' > .pr-body.txt
# Push the branch to GitHub and a PR
gh pr create --title "[r103] [TraceQL] Implement structural operators" --body-file .pr-body.txt --label "type/bug" --label "backport" --base r103 --milestone r103 --web

Or, if you don't have the GitHub CLI installed (we recommend you install it!):

# If you don't have the GitHub CLI installed: Push the branch to GitHub and manually create a PR:
git push --set-upstream origin backport-2625-to-r103
# Remove the local backport branch
git switch main
git branch -D backport-2625-to-r103

Unless you've used the GitHub CLI above, now create a pull request where the base branch is r103 and the compare/head branch is backport-2625-to-r103.

mapno pushed a commit to mapno/tempo that referenced this pull request Jul 14, 2023
* Dynamic metrics group by multiple attributes

* changelog

* review feedback

* trim whitespace on groupBy values

* Working descendant operator

* Add child and sibling operators

* Split intrinsic into separate per operator to allow better column precision. Fix edge case, fix flakey tests

* lint

* changelog

* Add missing default scope entries

* Fix dual import of parquetquery

* Fail spanset structural if multiple spansets present, add tests, use buffer

* Document structural operators

* Review feedback, comments

* lint

* lint

(cherry picked from commit ac63a44)
@mapno mapno mentioned this pull request Jul 14, 2023
mapno added a commit that referenced this pull request Jul 14, 2023
* Dynamic metrics group by multiple attributes

* changelog

* review feedback

* trim whitespace on groupBy values

* Working descendant operator

* Add child and sibling operators

* Split intrinsic into separate per operator to allow better column precision. Fix edge case, fix flakey tests

* lint

* changelog

* Add missing default scope entries

* Fix dual import of parquetquery

* Fail spanset structural if multiple spansets present, add tests, use buffer

* Document structural operators

* Review feedback, comments

* lint

* lint

(cherry picked from commit ac63a44)

Co-authored-by: Martin Disibio <[email protected]>
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