-
Notifications
You must be signed in to change notification settings - Fork 50
Filtering: NonApplicability improvements #1373
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
ab898b3
7413638
33b4091
bea96d7
e6e7296
3f5d04a
25b514e
5c146bc
b24b42f
90de9f9
d510a18
22231aa
76582e9
1ea1e59
5d7ed6e
0b5806a
cfbfa10
78beba4
d7f4cb5
dc5fd3c
cdae7e7
6723f29
e32af32
65b52e4
200f5fc
5769a1f
7543750
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,10 @@ | ||
| import { | ||
| DataSourceApi, | ||
| // @ts-expect-error (temporary till we update grafana/data) | ||
| DrilldownsApplicability, | ||
| Scope, | ||
| TimeRange, | ||
| } from '@grafana/data'; | ||
| import { | ||
| findClosestAdHocFilterInHierarchy, | ||
| findGlobalAdHocFilterVariableByUid, | ||
|
|
@@ -14,7 +21,7 @@ import { | |
| isFilterComplete, | ||
| } from '../variables/adhoc/AdHocFiltersVariable'; | ||
| import { VariableDependencyConfig } from '../variables/VariableDependencyConfig'; | ||
| import { SceneObject, SceneObjectState } from '../core/types'; | ||
| import { SceneDataQuery, SceneObject, SceneObjectState } from '../core/types'; | ||
|
|
||
| /** | ||
| * Manages ad-hoc filters and group-by variables for data providers | ||
|
|
@@ -23,6 +30,7 @@ export class DrilldownDependenciesManager<TState extends SceneObjectState> { | |
| private _adhocFiltersVar?: AdHocFiltersVariable; | ||
| private _groupByVar?: GroupByVariable; | ||
| private _variableDependency: VariableDependencyConfig<TState>; | ||
| private _applicabilityResults?: DrilldownsApplicability[]; | ||
|
|
||
| public constructor(variableDependency: VariableDependencyConfig<TState>) { | ||
| this._variableDependency = variableDependency; | ||
|
|
@@ -80,16 +88,129 @@ export class DrilldownDependenciesManager<TState extends SceneObjectState> { | |
| return this._groupByVar; | ||
| } | ||
|
|
||
| /** | ||
| * Resolves per-panel drilldown applicability before the data query runs. | ||
| * All gating logic lives here so SceneQueryRunner stays thin. | ||
| */ | ||
| public async resolveApplicability( | ||
| ds: DataSourceApi, | ||
| queries: SceneDataQuery[], | ||
| timeRange: TimeRange, | ||
| scopes: Scope[] | undefined | ||
| ): Promise<void> { | ||
| if (!this._adhocFiltersVar && !this._groupByVar) { | ||
| return; | ||
| } | ||
|
|
||
| // @ts-expect-error (temporary till we update grafana/data) | ||
| if (!ds.getDrilldownsApplicability) { | ||
| return; | ||
| } | ||
|
|
||
| const filters = this._adhocFiltersVar | ||
| ? [...this._adhocFiltersVar.state.filters, ...(this._adhocFiltersVar.state.originFilters ?? [])] | ||
| : []; | ||
| const groupByKeys = this._groupByVar | ||
| ? Array.isArray(this._groupByVar.state.value) | ||
| ? this._groupByVar.state.value.map((v) => String(v)) | ||
| : this._groupByVar.state.value | ||
| ? [String(this._groupByVar.state.value)] | ||
| : [] | ||
| : []; | ||
|
|
||
| if (filters.length === 0 && groupByKeys.length === 0) { | ||
| this._clearPerPanelApplicability(); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // @ts-expect-error (temporary till we update grafana/data) | ||
| const results = await ds.getDrilldownsApplicability({ | ||
| filters, | ||
| groupByKeys, | ||
| queries, | ||
| timeRange, | ||
| scopes, | ||
| }); | ||
| this._setPerPanelApplicability(results); | ||
| } catch { | ||
| this._clearPerPanelApplicability(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the raw applicability results from the last resolveApplicability() call. | ||
| * Used by UI components to display which filters are non-applicable for this panel. | ||
| */ | ||
| public getApplicabilityResults(): DrilldownsApplicability[] | undefined { | ||
| return this._applicabilityResults; | ||
| } | ||
|
|
||
| private _clearPerPanelApplicability(): void { | ||
| this._applicabilityResults = undefined; | ||
| } | ||
|
|
||
| private _setPerPanelApplicability(results: DrilldownsApplicability[]): void { | ||
| this._applicabilityResults = results; | ||
| } | ||
|
|
||
| public getFilters(): AdHocFilterWithLabels[] | undefined { | ||
| return this._adhocFiltersVar | ||
| ? [...(this._adhocFiltersVar.state.originFilters ?? []), ...this._adhocFiltersVar.state.filters].filter( | ||
| (f) => isFilterComplete(f) && isFilterApplicable(f) | ||
| ) | ||
| : undefined; | ||
| if (!this._adhocFiltersVar) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this entire manager will get very thin after the unification, basically it would just deal with adhoc subscription and fetching filters... might not even need it at all we'll see |
||
| return undefined; | ||
| } | ||
|
|
||
| const stateFilters = this._adhocFiltersVar.state.filters; | ||
| const originFilters = this._adhocFiltersVar.state.originFilters ?? []; | ||
|
|
||
| // Reconstruct sent indices: resolveApplicability sends [...stateFilters, ...originFilters] | ||
| const allWithIndex: Array<{ filter: AdHocFilterWithLabels; sentIndex: number }> = [ | ||
| ...originFilters.map((f, i) => ({ filter: f, sentIndex: stateFilters.length + i })), | ||
| ...stateFilters.map((f, i) => ({ filter: f, sentIndex: i })), | ||
| ].filter(({ filter: f }) => isFilterComplete(f) && isFilterApplicable(f)); | ||
|
|
||
| if (!this._applicabilityResults) { | ||
| return allWithIndex.map(({ filter }) => filter); | ||
| } | ||
|
|
||
| return allWithIndex | ||
| .filter(({ sentIndex }) => { | ||
| if (sentIndex >= this._applicabilityResults!.length) { | ||
| return true; | ||
| } | ||
| return this._applicabilityResults![sentIndex].applicable; | ||
| }) | ||
| .map(({ filter }) => filter); | ||
| } | ||
|
|
||
| public getGroupByKeys(): string[] | undefined { | ||
| return this._groupByVar ? this._groupByVar.getApplicableKeys() : undefined; | ||
| if (!this._groupByVar) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const keys = this._groupByVar.getApplicableKeys(); | ||
|
|
||
| if (!this._applicabilityResults) { | ||
| return keys; | ||
| } | ||
|
|
||
| // Rebuild the full sent groupByKeys to find each key's sent position | ||
| const val = this._groupByVar.state.value; | ||
| const allGroupByKeys = Array.isArray(val) ? val.map(String) : val ? [String(val)] : []; | ||
| const filtersCount = this._adhocFiltersVar | ||
| ? this._adhocFiltersVar.state.filters.length + (this._adhocFiltersVar.state.originFilters?.length ?? 0) | ||
| : 0; | ||
|
|
||
| return keys.filter((k) => { | ||
| const sentIdx = allGroupByKeys.indexOf(k); | ||
| if (sentIdx === -1) { | ||
| return true; | ||
| } | ||
| const resultIdx = filtersCount + sentIdx; | ||
| if (resultIdx >= this._applicabilityResults!.length) { | ||
| return true; | ||
| } | ||
| return this._applicabilityResults![resultIdx].applicable; | ||
| }); | ||
| } | ||
|
|
||
| public cleanup(): void { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -744,39 +744,34 @@ export class AdHocFiltersVariable | |
| return; | ||
| } | ||
|
|
||
| const responseMap = new Map<string, DrilldownsApplicability>(); | ||
| response.forEach((filter: DrilldownsApplicability) => { | ||
| responseMap.set(`${filter.key}${filter.origin ? `-${filter.origin}` : ''}`, filter); | ||
| }); | ||
|
|
||
| const update = { | ||
| applicabilityEnabled: true, | ||
| filters: [...this.state.filters], | ||
| originFilters: [...(this.state.originFilters ?? [])], | ||
| }; | ||
|
|
||
| update.filters.forEach((f) => { | ||
| const filter = responseMap.get(f.key); | ||
| const filtersCount = update.filters.length; | ||
|
|
||
| if (filter) { | ||
| f.nonApplicable = !filter.applicable; | ||
| f.nonApplicableReason = filter.reason; | ||
| update.filters.forEach((f, i) => { | ||
| if (i < response.length) { | ||
| f.nonApplicable = !response[i].applicable; | ||
| f.nonApplicableReason = response[i].reason; | ||
| } | ||
| }); | ||
|
|
||
| update.originFilters?.forEach((f) => { | ||
| const filter = responseMap.get(`${f.key}-${f.origin}`); | ||
|
|
||
| if (filter) { | ||
| update.originFilters?.forEach((f, i) => { | ||
| const responseIdx = filtersCount + i; | ||
| const result = responseIdx < response.length ? response[responseIdx] : undefined; | ||
| if (result) { | ||
| if (!f.matchAllFilter) { | ||
| f.nonApplicable = !filter.applicable; | ||
| f.nonApplicableReason = filter.reason; | ||
| f.nonApplicable = !result.applicable; | ||
| f.nonApplicableReason = result.reason; | ||
| } | ||
|
|
||
| const originalValue = this._originalValues.get(`${f.key}-${f.origin}`); | ||
| if (originalValue) { | ||
| originalValue.nonApplicable = !filter.applicable; | ||
| originalValue.nonApplicableReason = filter?.reason; | ||
| originalValue.nonApplicable = !result.applicable; | ||
| originalValue.nonApplicableReason = result?.reason; | ||
| } | ||
| } | ||
| }); | ||
|
|
@@ -851,9 +846,10 @@ export class AdHocFiltersVariable | |
| return []; | ||
| } | ||
|
|
||
| const originFilters = this.state.originFilters?.filter((f) => f.key !== filter.key) ?? []; | ||
| // Filter out the current filter key from the list of all filters | ||
| const otherFilters = this.state.filters.filter((f) => f.key !== filter.key).concat(originFilters); | ||
| const originFilters = this.state.originFilters?.filter((f) => f.key !== filter.key && !f.nonApplicable) ?? []; | ||
| const otherFilters = this.state.filters | ||
| .filter((f) => f.key !== filter.key && !f.nonApplicable) | ||
| .concat(originFilters); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to check in values too for non applicability, otherwise if we have some nonapplicable filters and search for values, nothing will come up because of that nonexistent label being set on the query |
||
|
|
||
| const timeRange = sceneGraph.getTimeRange(this).state.value; | ||
| const queries = this.state.useQueriesAsFilterForOptions ? getQueriesForVariables(this) : undefined; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to await here? This is gonna block query execution. Maybe instead just fire this funtion without await.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could let it fire and not block querying but then we would cancel inflights and requuery as soon as applicability resolves so we get the correct filters for the given queries