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

Tweak traceql parsing to correctly parse binary operations without space #1941

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented Dec 7, 2022

What this PR does:
This PR changes parsing of most binary operations to work without spacing. Example: {.foo="bar"}. Previously this wasn't handled as expected and the entire foo="bar" was considered the attribute name. This was done purposefully to accommodate searching on attribute names with syntaxy characters like = ~ - + etc, but the negative impact to every day usage isn't worth it.

Characters in the changed operations can no longer be included in an attribute name so querying on them is impossible. Some operations are not changed because the likelihood of those characters in attribute names is enough to weigh the decision the other way.

Fixed: = != =~ != > >= < <= ^ && ||
Unchanged: + - / % *

Overall
Currently this isn't in a good spot because we are trading parsing flexibility for attribute name support. TraceQL needs some escape hatches to support the full range of valid attribute names which is any valid unicode sequence and could include things like newline, space, quotes, etc. There's no way to unambiguously fit this into the current language spec.

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

Checklist

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

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 really want to add all of the operators + - % * /, but I recognize that - in particular is likely to be somewhere. I suppose there's some consistency in that the arithmetic operators require spaces but the binary operators don't?

wdyt?

@mdisibio
Copy link
Contributor Author

mdisibio commented Dec 7, 2022

I really want to add all of the operators + - % * /, but I recognize that - in particular is likely to be somewhere. I suppose there's some consistency in that the arithmetic operators require spaces but the binary operators don't?

wdyt?

Not opposed. I suppose restricting behavior at the beginning is usually the best because it's easier to relax specifications later (thinking about if we were revert this change down the road and restore support for - it would silently break existing queries). In that sense adding all the operators does seem the best?

@joe-elliott
Copy link
Member

Checking internal data as expected most of this list does not show up tag names.
/ shows up in 2:
image

and - predictably shows up in a large number of attributes. I would guess nearly 100 total. Most of which are http header attributes:
image

In my mind it's more likely that someone will query an attribute with - in the name then use the arithmetic operators and I want the best experience out of the gate. I think we go with this as is and figure it out on the other side.

@mdisibio mdisibio merged commit e9b4391 into grafana:main Dec 7, 2022
@mdisibio mdisibio mentioned this pull request Jan 13, 2023
9 tasks
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.

TraceQL not finding matches in some circumstances
2 participants