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

Tempo: Upgrade @grafana/lezer-traceql patch version to use trace metrics syntax #82532

Merged
merged 3 commits into from Feb 16, 2024

Conversation

joey-grafana
Copy link
Contributor

What is this feature?

Upgrades @grafana/lezer-traceql patch version to use trace metrics syntax.

Why do we need this feature?

Add syntax support for trace metrics queries, introduced in this PR.

Who is this feature for?

Tempo users.

Special notes for your reviewer:

Screenshot 2024-02-15 at 11 37 28

@joey-grafana joey-grafana added this to the 10.4.x milestone Feb 15, 2024
@joey-grafana joey-grafana self-assigned this Feb 15, 2024
@joey-grafana joey-grafana marked this pull request as ready for review February 15, 2024 11:41
@joey-grafana joey-grafana requested review from a team as code owners February 15, 2024 11:41
@joey-grafana joey-grafana requested review from ashharrison90 and removed request for a team and ashharrison90 February 15, 2024 11:41
@joey-grafana
Copy link
Contributor Author

So it looks like after the updates to include metrics syntax, we exposed an existing issue that was incorrectly building the tree for by() and select() operations. Apparently they were aggregate functions because the tree was built in a way that suggested an empty by() or select() actually had a FieldExpression -> IntrinsicAttribute below it. This does not seem correct. If the operation is empty but should have args then the error should be directly below the that node in the tree.

Anyway, updated the PR to reflect a more specific error message and to also update the autocomplete for the by / select operations - i.e. continue to suggest attributes.

If you wouldn't mind taking another look @adrapereira / @fabrizio-grafana that would be great!

Note I plan to add more specific highlighting / possibly autocomplete for the new syntax in a subsequent PR.

Before
Screenshot 2024-02-15 at 15 20 50

After
Screenshot 2024-02-15 at 15 21 00

@joey-grafana
Copy link
Contributor Author

Ohh I should note that I'm not 100% sure why adding in the metrics syntax caused a change in how the tree is build. My guess is some kind of precedence but not certain. For context: I believe it was the lines with the MetricArgs that caused the change.

@fabrizio-grafana
Copy link
Contributor

Ohh I should note that I'm not 100% sure why adding in the metrics syntax caused a change in how the tree is build. My guess is some kind of precedence but not certain. For context: I believe it was the lines with the MetricArgs that caused the change.

Yeah probably, IIRC it happened also to me, but it shouldn't be a problem. In fact, I think we should also rethink tests in the lezer-traceql repo, since they check that the tree structure doesn't change, but IMO that is an implementation detail, since we don't really care about it, as long as queries are still parsed as expected. We can discuss this better live.

@@ -1028,6 +1028,12 @@
"count": 5
}
],
"/packages/grafana-ui/src/components/Splitter/Splitter.tsx": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Are changes to .betterer.results.json intended? I don't understand them 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like these were left over from other PR's that I merged in from main. Not related to this PR but a side effect of making a commit. Honestly not sure why.

@joey-grafana joey-grafana merged commit 1744487 into main Feb 16, 2024
14 checks passed
@joey-grafana joey-grafana deleted the joey/tempo-upgrade-lezer-traceql-package branch February 16, 2024 11:17
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants