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: Integrate context aware autocomplete API #67845

Merged
merged 5 commits into from
Aug 11, 2023

Conversation

adrapereira
Copy link
Contributor

What is this feature?

This PR integrates the new context aware autocomplete API from Tempo, by sending the full query in the q parameter of the api/v2/search/tags/.../values endpoint.

It also removes some unused code in the Tempo language provider.

Why do we need this feature?

We want to use the existing query to filter down the possible values for new tags and help the users write valid queries faster.

Who is this feature for?

All users of the Tempo datasource.

Which issue(s) does this PR fix?:

Fixes #66956

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@grafanabot
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@grafanabot grafanabot added stale Issue with no recent activity and removed stale Issue with no recent activity labels Jun 4, 2023
@grafanabot
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@grafanabot grafanabot added stale Issue with no recent activity and removed stale Issue with no recent activity labels Jul 5, 2023
@09jvilla 09jvilla modified the milestones: 10.1.x, 10.2.x Aug 1, 2023
…utocomplete_traceql_api

# Conflicts:
#	public/app/plugins/datasource/tempo/SearchTraceQLEditor/SearchField.tsx
#	public/app/plugins/datasource/tempo/language_provider.ts
#	public/app/plugins/datasource/tempo/traceql/autocomplete.ts
@adrapereira
Copy link
Contributor Author

@grafana/observability-traces-and-profiling Would appreciate a review!

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.

Noticed that the API uses double quotes around the query but we do not here. I'd imagine that's an older example of the API that has since been updated so probably no issue.

Overall I like what the API is doing, however there is a use case where the user has created a query that returns few values or no values for a tag, and the user is not quite sure why. If there are no values we show No options found in the drop down. I thin this can be improved in another PR to make a note of sorts so the user knows that removing parts of their query could result in more options.

public/app/plugins/datasource/tempo/traceql/situation.ts Outdated Show resolved Hide resolved
public/app/plugins/datasource/tempo/traceql/situation.ts Outdated Show resolved Hide resolved
public/app/plugins/datasource/tempo/traceql/situation.ts Outdated Show resolved Hide resolved
@adrapereira
Copy link
Contributor Author

Thanks for the review @joey-grafana! Yeah, I tested this against the API and it was working without quotes, but thank you for bringing it up.

That's a good idea, it's not immediately clear that there are no options because of the context around it. I'll create an issue for it.

@adrapereira adrapereira merged commit 039ed7a into main Aug 11, 2023
15 checks passed
@adrapereira adrapereira deleted the andre/integrate_new_autocomplete_traceql_api branch August 11, 2023 09:31
aishyandapalli pushed a commit to aishyandapalli/grafana that referenced this pull request Aug 16, 2023
* Send query in search tag values call

* Make sure to send the full query when using the code editor

* Fix merge conflicts

* Remove unused params
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
* Send query in search tag values call

* Make sure to send the full query when using the code editor

* Fix merge conflicts

* Remove unused params
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 - Integrate context aware autocomplete
5 participants