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: Allow quotes in tag names and attributes #77864

Merged
merged 6 commits into from
Nov 24, 2023

Conversation

fabrizio-grafana
Copy link
Contributor

@fabrizio-grafana fabrizio-grafana commented Nov 8, 2023

Allow tag names and values to use ". Examples:
image
image

Note that this allows to write queries that might look wrong at first sight. Example (note the missing closing double quote after GET):
image

Also, note that for autocompletion to work properly, we need a corresponding fix in Tempo, as mentioned here.

Fixes #77394
Fixes #77685

@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.3.x milestone Nov 8, 2023
@fabrizio-grafana fabrizio-grafana self-assigned this Nov 8, 2023
@fabrizio-grafana fabrizio-grafana marked this pull request as ready for review November 8, 2023 12:45
@fabrizio-grafana fabrizio-grafana requested review from a team as code owners November 8, 2023 12:45
@fabrizio-grafana fabrizio-grafana requested review from academo and removed request for a team November 8, 2023 12:45
@fabrizio-grafana
Copy link
Contributor Author

@joe-elliott Tagging you just to be sure we are on the same page that allowing to use quotes for attribute names can also allow for some weird tag names and thus queries (see example in the PR description). I think this is a caveat we cannot avoid and that it is up to the user to avoid to abuse of this feature (usage of quotes in attribute names). But if you have better ideas, happy to hear them!

@joe-elliott
Copy link
Member

Yup, this definitely makes some horrible things possible :). This is fine because if you are naming your tags:

he\ll o world $ = GET

Then it's ok that your TraceQL queries look unreadable.

Copy link
Contributor

@joey-grafana joey-grafana left a comment

Choose a reason for hiding this comment

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

Could we get a more detailed error message for this?

Screenshot 2023-11-15 at 20 31 16

Also, honestly I'm not sure why we need to support some of these strange strings with quotes all over the place :D Is it because the quotes can technically be anywhere in the string and we are not sure of all the cases where it would be invalid/valid so it would be difficult to restrict where the quotes can be? (so we just basically allow the quotes in many places)

@fabrizio-grafana
Copy link
Contributor Author

fabrizio-grafana commented Nov 16, 2023

Could we get a more detailed error message for this?

Screenshot 2023-11-15 at 20 31 16 Also, honestly I'm not sure why we need to support some of these strange strings with quotes all over the place :D Is it because the quotes can technically be anywhere in the string and we are not sure of all the cases where it would be invalid/valid so it would be difficult to restrict where the quotes can be? (so we just basically allow the quotes in many places)

We can discuss this, also with the Tempo squad. For now I've just implemented the original request and according to what Tempo allows, namely quotes at any point:
image

@fabrizio-grafana
Copy link
Contributor Author

Discussed offline. We agreed to allow quotes as in Tempo, at least for the moment.

@fabrizio-grafana fabrizio-grafana requested review from joey-grafana and removed request for academo November 24, 2023 14:59
Copy link
Contributor

@joey-grafana joey-grafana left a comment

Choose a reason for hiding this comment

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

LGTM 👍

public/app/plugins/datasource/tempo/traceql/traceql.ts Outdated Show resolved Hide resolved
@fabrizio-grafana fabrizio-grafana merged commit 6c7beb1 into main Nov 24, 2023
16 checks passed
@fabrizio-grafana fabrizio-grafana deleted the otp/traceql-quoted-attributes branch November 24, 2023 16:24
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
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.

[TraceQL] Support escaping in attribute values [TraceQL] Support quoted attribute names
4 participants