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.

Sign up for GitHub

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

Jump to bottom

AdHocFiltersVariable/GroupByVariable: Pass time range to getTagKeys calls #665

Merged
merged 3 commits into from
Apr 2, 2024
Merged

AdHocFiltersVariable/GroupByVariable: Pass time range to getTagKeys calls #665

merged 3 commits into from
Apr 2, 2024

Conversation

bfmatei
Copy link
Contributor

@bfmatei bfmatei commented Apr 1, 2024 •
edited by grafanabot

Time range is not passed to getTagKeys calls currently but we actually need it to be passed.

📦 Published PR as canary version: 4.1.1--canary.665.8521264218.0

✨ Test out this PR locally via:

npm install @grafana/scenes@4.1.1--canary.665.8521264218.0
# or 
yarn add @grafana/scenes@4.1.1--canary.665.8521264218.0

@dprokop
Copy link
Member

dprokop commented Apr 2, 2024

Looks good, but given that both DataSourceGetTagKeysOptions and DataSourceGetTagValuesOptions accept time range as an option, can we provide the time range to the getTagKeys calls in group by variable and getTagValues in the ad hoc filter?

@dprokop dprokop added patch Increment the patch version when merged release Create a release when this pr is merged labels Apr 2, 2024
ashharrison90
ashharrison90 reviewed Apr 2, 2024
View reviewed changes
Copy link
Contributor

@ashharrison90 ashharrison90 left a comment •
edited

Choose a reason for hiding this comment

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

+1 to dominik's comment, let's make sure it's passed everywhere else we're just gonna have a future request to add it. so:

  • adhoc filters getTagKeys ✅
  • adhoc filters getTagValues ✅
  • groupby getTagKeys ✅
  • groupby getTagValues never called

packages/scenes/src/variables/adhoc/AdHocFiltersVariable.tsx
@@ -203,9 +203,10 @@ export class AdHocFiltersVariable
}

const otherFilters = this.state.filters.filter((f) => f.key !== currentKey).concat(this.state.baseFilters ?? []);
const timeRange = sceneGraph.getTimeRange(this).state.value;
const queries = this._getSceneQueries();
// @ts-expect-error TODO: remove this once 10.4.0 is released
Copy link
Contributor

Choose a reason for hiding this comment

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

not strictly related to this PR, but can we remove this TODO now? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌 👌 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LE: Nope. Updating @grafana/ packages to 10.4.x actually breaks a lot of tests. We'll need another PR specific to that.

@bfmatei bfmatei changed the title AdHocFiltersVariable: Pass time range to getTagKeys calls AdHocFiltersVariable/GroupByVariable: Pass time range to getTagKeys calls Apr 2, 2024
@bfmatei
Copy link
Contributor Author

bfmatei commented Apr 2, 2024

@ashharrison90 @dprokop - getTagValues already had the time range passed in AdHocFiltersVariable. I added it to getTagKeys in GroupByVariable as well.

The only question is: are we calling getTagValues in GroupByVariable? I can't find any reference to it.

@bfmatei bfmatei force-pushed the bogdan/adhoc-filters-pass-timerange-gettagkeys branch from 6d8b086 to db36798 Compare April 2, 2024 10:25
@ashharrison90
Copy link
Contributor

@bfmatei ah perfect, didn't even notice the timeRange being passed in adhoc filters getTagValues 🥳

and no, i don't think we ever call getTagValues in group by. i'll update my comment now 👍

ashharrison90
ashharrison90 approved these changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@bfmatei bfmatei merged commit c5a1dfe into main Apr 2, 2024
3 checks passed
@bfmatei bfmatei deleted the bogdan/adhoc-filters-pass-timerange-gettagkeys branch April 2, 2024 11:28
@grafanabot
Copy link
Contributor

🚀 PR was released in v4.1.2 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashharrison90 ashharrison90 ashharrison90 approved these changes

@torkelo torkelo Awaiting requested review from torkelo

@dprokop dprokop Awaiting requested review from dprokop

Assignees

@bfmatei bfmatei

Labels
patch Increment the patch version when merged release Create a release when this pr is merged released
Projects
Dashboards
Status: 🚀 Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bfmatei @dprokop @ashharrison90 @grafanabot