AdHocFilters: Add support for new isOneOf multi value operator
#868
Conversation
| @@ -40,6 +40,7 @@ export function getAdhocFiltersDemo(defaults: SceneAppPageState) { | |||
| // Only want keys for this series | |||
| baseFilters: [{ key: '__name__', operator: '=', value: 'ALERTS', condition: '' }], | |||
| datasource: { uid: 'gdev-prometheus' }, | |||
| supportsMultiValueOperators: true, | |||
There was a problem hiding this comment.
won't support for this be dependent on each data source, so not sure how we can set this in core dashboards. would have to update it's value after we load the datasource plugin and check if it supports it
There was a problem hiding this comment.
hmmmmm, good point. i think we were trying to avoid putting something like that in the datasource since then we'd have to wait for the datasource request to return.
we could just put all this logic behind the feature toggle, and implement support in the other datasources before enabling the toggle? 🤔
looking at the docs, it looks like adhoc filters are only supported in Prometheus, Loki, InfluxDB, and Elasticsearch.
prometheus support is added in grafana#91837, can't imagine it would be much work to add support in loki/influx/elastic.
There was a problem hiding this comment.
Given that we are going to introduce this behind a feature toggle I think we could assume it's only Prometheus that supports multi value filters by default from day one. Tho I agree with @ashharrison90 that adding support for this in the data sources that already support ad hoc filters shouldn't be too much of an effort.
IMO whether or not data source should allow this operator would eventually depend on a different API (type-aware filters) where the availability of operators would be driven by getTagKeys call results, so that each dimension would define what type it is (string, number, enum) and whether or not it supports multi value selection.
WDYT?
There was a problem hiding this comment.
i think that's fine - the question then becomes what should we do as part of this PR/iteration? 🤔
e.g. is the implementation in this PR and grafana/grafana#91837 ok? with the understanding that i can add support in loki/influx/elastic datasources in follow up PRs. given they all should support the regex operator it should be trivial to do something similar as what we've done for prometheus.
There was a problem hiding this comment.
@ashharrison90 - really great progress on this. I've left couple of comments in the code, and here are some more UX/UI related things:
- With multi value filter the filter is applied as soon as you choose an individual value currently. This results in queries being performed as you build the filter. I think we need to align this behavior with multi value variable so that the queries are executed when the user selected all desired values. That would be on filter value input blur.
- Removing individual value by clicking (x) on the pill results in the filter value popover being shown - i think we need to stop the events propagation.
@Sergej-Vlasov - please keep an eyer and review this PR as we need to integrate this work with the work you are doing in #830
| }, | ||
| closeMenuOnSelect: false, | ||
| onChange: (v: SelectableValue) => { | ||
| const updatedValue = v.map((option: SelectableValue<string>) => option.value).join('__gfp__'); |
There was a problem hiding this comment.
What do you think about keeping the value as a regex string rather than joining individual values with a special set of characters? I think that would be more intuitive.
There was a problem hiding this comment.
tbf i don't think this even really gets used at all, since the datasource looks for the presence of values...
we could just set it to the first value in the values array? 🤔
we could also make it the regex string. i guess that would mean we have to join the values with | but also regex escape the individual values
ashharrison90
added
minor
There was a problem hiding this comment.
I tested this through grafana/grafana#91837 and it seems to work fine
|
🚀 PR was released in |
supportsMultiValueOperatorproperty toAdHocFiltersVariableisMultiValueOperatormethod_getOperatorsto conditionally return the new multi value operators depending on the value ofsupportsMultiValueOperator_updateFilterto take in a generic partial filter update<MultiSelect>when a multi value operator is selected inAdHocFilterRendererrenderFilterexpression (which should produce a prometheus-compatible expression) to handle=|and!=|by converting to regex and joining with|AdHocFiltersVariableUrlSyncHandlerto handle multiple value/label pairsvalueLabeltovalueLabelsso it can hold multiple labelsFor https://github.com/grafana/hyperion-planning/issues/1
For grafana/grafana#85074
📦 Published PR as canary version:
5.11.0--canary.868.10613389161.0✨ Test out this PR locally via:
npm install @grafana/scenes-react@5.11.0--canary.868.10613389161.0 npm install @grafana/scenes@5.11.0--canary.868.10613389161.0 # or yarn add @grafana/scenes-react@5.11.0--canary.868.10613389161.0 yarn add @grafana/scenes@5.11.0--canary.868.10613389161.0