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

Variables: Interpolate datasource uid when used with datasource variable #996

Merged
merged 4 commits into from
Dec 18, 2024

Conversation

Sergej-Vlasov
Copy link
Contributor

@Sergej-Vlasov Sergej-Vlasov commented Dec 9, 2024

Use case - use datasource variable as a datasource for ad hoc filter and in a dashboard query.
so we have datasource variable dsVar and it equals to prom-1 as example then:

  1. ds variable in filter and panel (both ${dsVar} )
  2. ds variable in filter (${dsVar}) and named ds in panel (prom-1) that are the same
  3. named variable in filter (prom-1) and ds variable in panel (${dsVar}) that are the same

Issues solved:

  1. findAndSubscribeToAdHocFilters did not get interpolated datasource name.
  2. ad hoc filter options would not filter down options based on query in dashboard therefore interpolated datasource uuids in getQueriesForVariables

adding dashboard JSON that replicates issue:
dsVar as filter datasource-1733769038417.json

📦 Published PR as canary version: 5.33.1--canary.996.12371059735.0

✨ Test out this PR locally via:

npm install @grafana/[email protected]
npm install @grafana/[email protected]
# or 
yarn add @grafana/[email protected]
yarn add @grafana/[email protected]

@Sergej-Vlasov Sergej-Vlasov added the area/variables Issues related to variables system in scenes label Dec 9, 2024
@Sergej-Vlasov Sergej-Vlasov self-assigned this Dec 9, 2024
this.findAndSubscribeToAdHocFilters(datasource?.uid);
this.findAndSubscribeToAdHocFilters(ds.uid);
Copy link
Member

Choose a reason for hiding this comment

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

this could break existing logic, needs to be the same that was specified in SceneQueryRunner config (the string that has the variable expression), here you are just passing in the uid

so if the ad-hoc filter has a variable expression as data source then this change will break that lookup

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think this mainly means that we must make sure that in findAndSubscribeToAdHocFilters both filters and query runner ds are interpolated when comparing. Not sure what would that break really, it just makes sure that the same data sources are used.

@Sergej-Vlasov Sergej-Vlasov marked this pull request as ready for review December 12, 2024 16:46
@Sergej-Vlasov Sergej-Vlasov added release Create a release when this pr is merged patch Increment the patch version when merged labels Dec 17, 2024
@Sergej-Vlasov Sergej-Vlasov merged commit a5e7915 into main Dec 18, 2024
5 checks passed
@Sergej-Vlasov Sergej-Vlasov deleted the serge/interpolate-ds-name-from-variable branch December 18, 2024 10:02
@scenes-repo-bot-access-token
Copy link

🚀 PR was released in v5.35.0 🚀

jest.resetAllMocks();
});

it.only('should get queries for interpolated source object and query datasource uuids', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Sergej-Vlasov stray .only here that needs removing. there is an eslint plugin (eslint-plugin-jest) and a corresponding rule (jest/no-focused-tests) that you should add as well to prevent this in the future 🙏

Copy link
Contributor Author

@Sergej-Vlasov Sergej-Vlasov Dec 18, 2024

Choose a reason for hiding this comment

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

good catch! Fix here #1008

Will follow up with the eslint-plugin-jest issue because it flags around 80 errors
captured issue here #1009

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/variables Issues related to variables system in scenes patch Increment the patch version when merged release Create a release when this pr is merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants