Conversation
| 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); |
There was a problem hiding this comment.
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 filtersApplicabilityEnabled = this._adhocFiltersVar?.state.applicabilityEnabled; | ||
| const groupByApplicabilityEnabled = this._groupByVar?.state.applicabilityEnabled; | ||
|
|
||
| if (!filtersApplicabilityEnabled && !groupByApplicabilityEnabled) { |
There was a problem hiding this comment.
this is basically the grafana FF check
dprokop
left a comment
There was a problem hiding this comment.
Good idea to centralize the logic. Please adress my comments before we continue.
|
|
||
| this._drilldownDependenciesManager.findAndSubscribeToDrilldowns(ds.uid, this); | ||
|
|
||
| await this._drilldownDependenciesManager.resolveApplicability( |
There was a problem hiding this comment.
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.
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
| } | ||
| }); | ||
|
|
||
| this.setState({ applicabilityEnabled: false }); |
There was a problem hiding this comment.
yes, that was causing some issues now that we set that prop through the ff in grafana
| export function buildApplicabilityCacheKey(parts: { | ||
| filters?: Array<{ origin?: string; key: string; operator: string; value: string; values?: string[] }>; | ||
| groupByKeys?: string[]; | ||
| value?: string[]; |
There was a problem hiding this comment.
we use this util to limit applicability calls -- if nothing changes, then we dont rerun the call with the same values basically
There was a problem hiding this comment.
oh nvm I think you're asking about the specific value param, I think that's not needed and I missed it, will doublecheck
|
|
||
| const stateFilters = this._adhocFiltersVar.state.filters; | ||
| const originFilters = this._adhocFiltersVar.state.originFilters ?? []; | ||
| const allFilters = [...originFilters, ...stateFilters]; |
There was a problem hiding this comment.
Sanity check - does it matter that in the adhoc filters we create const filters = [...this.state.filters, ...(this.state.originFilters ?? [])]; ? Reversed order
There was a problem hiding this comment.
good call, yeah this should be consistent and originfilters should be first by convention
There was a problem hiding this comment.
I've modified the adhoc to be consistent, the origin filters were put last mainly because it was easier and order didnt matter -- it still doesnt for origin ones vs non origin ones -- it matters mostly for same type of filters with same key (e.g. I have 2 user filters env=prod and then env=dev, the latter one should take priority and overwrite the first, this doesnt happen for origin and user ones because of the origin property delimitation)
| groupByKeys?: string[]; | ||
| value?: string[]; |
There was a problem hiding this comment.
why did you separate these two? IIUC these properties are used in the code to represent the same thing - group by keys.
There was a problem hiding this comment.
yes I think you are right, the value is unneeded
There was a problem hiding this comment.
removed value, unneeded
| } | ||
|
|
||
| function normalizeQuery(q: SceneDataQuery): { refId: string; expr?: unknown } { | ||
| return { refId: q.refId, expr: q.expr ?? q.expression ?? q.query }; |
There was a problem hiding this comment.
very prometheus-yyyyyy :)
There was a problem hiding this comment.
🤦🏼 focused so much on prometheus this slipped, I'm looking into it
| const json = store.get(this._getStorageKey()); | ||
| const storedGroupings = json ? JSON.parse(json) : []; | ||
|
|
||
| if (storedGroupings.length > 0) { |
There was a problem hiding this comment.
got rid of these extra requests for now since this will also be unified
| return applicableValues; | ||
| } | ||
|
|
||
| public async getGroupByApplicabilityForQueries( |
There was a problem hiding this comment.
same here, just removed functionality to have less requests, again will be solved by unification later
| return this._resolvedDs; | ||
| } | ||
|
|
||
| const ds = await this._dataSourceSrv.get(this.state.datasource, this._scopedVars); |
There was a problem hiding this comment.
reusing scopedVars from the adhoc in the recomendations as well ( through this method that is reused there), i reckon thats fine
| it('should add value to parent variable', async () => { | ||
| const getRecommendedDrilldowns = jest.fn().mockResolvedValue(undefined); | ||
| const getDrilldownsApplicability = jest.fn().mockResolvedValue(undefined); | ||
| const getDrilldownsApplicability = jest.fn().mockResolvedValue(new Map([['_default_', []]])); |
There was a problem hiding this comment.
at this point i think everything groupBy can be ignored, this will be removed after unification
| (f) => isFilterComplete(f) && isFilterApplicable(f) | ||
| ) | ||
| : undefined; | ||
| if (!this._adhocFiltersVar) { |
There was a problem hiding this comment.
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
Improves the filtering non-applicability system:
📦 Published PR as canary version:
7.2.0--canary.1373.22950463934.0✨ Test out this PR locally via:
npm install @grafana/scenes@7.2.0--canary.1373.22950463934.0 npm install @grafana/scenes-react@7.2.0--canary.1373.22950463934.0 # or yarn add @grafana/scenes@7.2.0--canary.1373.22950463934.0 yarn add @grafana/scenes-react@7.2.0--canary.1373.22950463934.0