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

Dashboard: Add support for Tempo query variables #72745

Merged
merged 20 commits into from
Aug 30, 2023

Conversation

fabrizio-grafana
Copy link
Contributor

@fabrizio-grafana fabrizio-grafana commented Aug 2, 2023

What is this feature?
Add support for template variables (Label names and Label values) in Tempo, similarly to what is already available in Loki and Prometheus. Quick demo.

Why do we need this feature?
It has been requested by a few customers and discussed with @09jvilla.

Who is this feature for?
Customers.

Which issue(s) does this PR fix?:
Fixes #72575
Fixes #70305

@fabrizio-grafana fabrizio-grafana self-assigned this Aug 2, 2023
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.2.x milestone Aug 2, 2023
@fabrizio-grafana fabrizio-grafana marked this pull request as ready for review August 2, 2023 14:42
@fabrizio-grafana fabrizio-grafana requested a review from a team as a code owner August 2, 2023 14:42
@fabrizio-grafana fabrizio-grafana changed the title Add support for Tempo template variables Dashboard: Add support for Tempo query variables Aug 3, 2023
Copy link
Contributor

@09jvilla 09jvilla left a comment

Choose a reason for hiding this comment

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

Didn't review the code but approving based on the functionality demonstrated. Amazing work!

We have discussed in a separate thread that we should create another PR that ensures Tempo query variables work when the dashboard panel query is made via the Search tab. Today they only work if the query is made via the TraceQL tab.

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 modify the fields here so that there is one per row? We could also shorten the label width here as they seem quite long. In contrast, I think we should make the second fields dropdown input a bit longer. As you can see in the image, it's quite short which makes it hard to see what was selected in the dropdown.

Screenshot 2023-08-18 at 14 54 36

public/app/plugins/datasource/tempo/datasource.ts Outdated Show resolved Hide resolved
public/app/plugins/datasource/tempo/mocks.ts Outdated Show resolved Hide resolved
@aocenas
Copy link
Member

aocenas commented Aug 21, 2023

@fabrizio-grafana did you talk with somebody regarding whether we need or don't need also a search result (so TraceIDs) as a template variable?

@09jvilla
Copy link
Contributor

@fabrizio-grafana did you talk with somebody regarding whether we need or don't need also a search result (so TraceIDs) as a template variable?

@aocenas -- This is something we're leaving out of scope for now and will revisit in the future if there is sufficient demand.

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.

This looks really good and will be a great addition :)

I've noticed something that we should fix before merging. Nothing major so should be ok to fix. Querying the v2 endpoint always returns an error because we are not providing the scope in the query.

i.e. the query is like api/v2/search/tag/job/values when it should be api/v2/search/tag/resource.job/values. As a result, we always fallback to v1 of the API. In this case I believe that tags result are the exact same so this still works, however we should probably fix the issue with the API v2 tags query as we will more than likely eventually drop support for v1 and move to v2 + it removes one error from the network tab/console.

Screenshot 2023-08-24 at 12 27 53

@fabrizio-grafana
Copy link
Contributor Author

Should be fixed!

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.

This looks great! 👍

@fabrizio-grafana fabrizio-grafana merged commit 5038137 into main Aug 30, 2023
15 checks passed
@fabrizio-grafana fabrizio-grafana deleted the template-variables branch August 30, 2023 11:45
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants