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

AdhocFilters: Improve typing and signature of getTagKeys and getTagValues and behaviors #74962

Merged
merged 8 commits into from
Sep 19, 2023

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Sep 15, 2023

Started working on adhoc filters for scenes and want to review and try to improve the system.

Goals

  • Enable adhoc filters to take into account existing added filters when suggesting keys and values
  • Enable adhoc filters to support a list of "baseFilters" that are not shown in the UI but used to filter down keys and values.
  • Enable data sources to get adhocFilters to apply to a query without having to call templateSrv.getAdhocFilters(this.name); (not in this PR)
  • Improve / add missing types

Changes

  • Add typing to getTagValues options argument
  • Add typing to getTagKeys options argument
  • Make options argument take in existing filters so that ad-hoc filters can take those into account when returning keys and values
  • Implement support for taking existing filters into account for Prometheus
  • Update AdhocFilter picker/builder so that it passes existing filters to getTagKeys and getTagValues
  • InfluxDB was also passing options arg to getTagKeys, with a very strange/incorrect typing (InfluxQuery).
  • Add some foundational support for "baseFilters", filters that are not visible in the filters UI but are taken into account when fetching filter keys and values. This replaces the custom Prometheus getTagKeys solution of passing in the series array.
  • Need help testing Tempo adhoc filter bit

The tempo => adhoc filters => Prometheus thing

To support adhoc filters in the tempo query builder a series[] name array was added to getTagKeys to filter tag keys by some series names. This is too specific to Prometheus. The getTagKeys and getTagValues need to be more generic. So changing this work with the new way of passing in existing filters / baseFilters. Prometheus can filter on multiple series names using regex label matching.

Adding adhocFilters to DataQueryRequest

The PR actually only started with this change, but then noticed the poor typing and flexibility of getTagKeys and getTagValues so switched the PR to addressing that.
So will move this change to a different PR.

Instead of data sources having to call templateSrv.getAdhocFilters(this.name); to get the relevant filters to apply to the queries I propose passing them via DataQueryRequest.

We would of course make templateSrv.getAdhocFilters(this.name) work for backward compatibility reasons, but going forward using filters passed via the request object is a lot more flexible and clean.

Some tricky questions:

  • Should we also add it to the backend query call? so technically the filters can be applied to the query on the backend. Otherwise adhoc filters will never work for public dashboards / secure querying.

Alternative

  • Add adhocFilters to DataQuery (so they are scoped to each query so you can technically issue queries in one call to DataSourceApi.query with only some of the queries having adhocFilters.

@torkelo torkelo added the type/discussion Issue to start a discussion label Sep 15, 2023
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.2.x milestone Sep 15, 2023
@github-actions github-actions bot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Sep 15, 2023
@grafana grafana deleted a comment from github-actions bot Sep 16, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 17, 2023

⚠️   Possible breaking changes

(Open the links below in a new tab to go to the correct steps)

grafana-data has possible breaking changes (more info)

Console output
Read our guideline

@torkelo torkelo changed the title DataSourceAPI: Add adhocFilters to DataQueryRequest DataSourceAPI: Improve typing and signature of getTagKeys and getTagValues Sep 18, 2023
@torkelo torkelo changed the title DataSourceAPI: Improve typing and signature of getTagKeys and getTagValues AdhocFillters: Improve typing and signature of getTagKeys and getTagValues and behaviors Sep 18, 2023
@torkelo torkelo changed the title AdhocFillters: Improve typing and signature of getTagKeys and getTagValues and behaviors AdhocFilters: Improve typing and signature of getTagKeys and getTagValues and behaviors Sep 18, 2023
@torkelo torkelo marked this pull request as ready for review September 18, 2023 10:15
@torkelo torkelo requested review from a team and grafanabot as code owners September 18, 2023 10:15
@torkelo torkelo requested review from axelavargas and removed request for a team September 18, 2023 10:15
@torkelo torkelo requested review from joshhunt, ashharrison90 and leventebalogh and removed request for a team, grafanabot, joshhunt and ashharrison90 September 18, 2023 10:15
Copy link
Contributor

@itsmylife itsmylife left a comment

Choose a reason for hiding this comment

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

LGTM with some questions

Copy link
Member

@aocenas aocenas left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Checked the Tempo service graph seems to work. Checked the other code but did not test prom or influx.

@torkelo torkelo added the no-backport Skip backport of PR label Sep 19, 2023
@torkelo torkelo merged commit 1105b93 into main Sep 19, 2023
21 of 22 checks passed
@torkelo torkelo deleted the ad-hoc-filters branch September 19, 2023 06:24
rwwiv pushed a commit that referenced this pull request Oct 2, 2023
…lues and behaviors (#74962)

* Add adhocFilters to DataQueryRequest

* More changes

* Progress

* Working

* added baseFilters to picker

* Remove unused code

* minor fix
@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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants