grafana / scenes

Build Grafana dashboards directly in your Grafana app plugins.
https://grafana.com/developers/scenes
Apache License 2.0
141 stars 21 forks source link

AdHocFiltersVariable: Pass scene reference to expressionBuilder #949

Closed gtk-grafana closed 2 weeks ago

gtk-grafana commented 2 weeks ago

What: This PR allows scene developers to use application state to modify how ad hoc variable expressions are rendered. In order to avoid regression, a new state property buildExpressionOnActivate was added which will alter the behavior of AdHocFiltersVariable to wait until activation to build the filterExpression. This allows a reference to the AdHocFiltersVariable can be included in the expressionBuilder callback which is not available currently when the filterExpression is created during instantiation.

Why: Currently the expressionBuilder method is unable to reference the application context, and use-cases where application state can influence how a particular ad hoc variable is interpolated is not supported.

How: Specifically, for Explore Logs, we need to conditionally ignore filters that match the label in the active tab so users can include multiple values by clicking on buttons in the result panels for a single label without needing to modify the ad-hoc variable state, which would introduce undesirable implications in the UI.

An example implementation in the app-plugin could look like the following:

  const labelVariable = new AdHocFiltersVariable({
    name: VAR_LABELS,
    datasource: EXPLORATION_DS,
    layout: 'combobox',
    buildExpressionOnActivate: true,
    expressionBuilder: renderLogQLLabelFilters,
   // [...]
  });

function expressionBuilder(filters: AdHocVariableFilter[], scene: SceneObject) {
  // Get the key of another ad hoc variable
  const primaryLabel = getServiceSelectionPrimaryLabel(scene).state.filters[0]?.key

  // Exclude filters matching the first key in the other variable
  filters = filters.filter(f => f.key !== primaryLabel)
  let { positiveFilters, negative } = getLogQLLabelFilters(filters);
  const negativeFilters = negative.map((filter) => renderMetadata(filter)).join(', ');

  return trim(`${positiveFilters.join(', ')}, ${negativeFilters}`, ' ,');
}
📦 Published PR as canary version: 5.21.2--canary.949.11612383247.0
:sparkles: Test out this PR locally via: ```bash npm install @grafana/scenes-react@5.21.2--canary.949.11612383247.0 npm install @grafana/scenes@5.21.2--canary.949.11612383247.0 # or yarn add @grafana/scenes-react@5.21.2--canary.949.11612383247.0 yarn add @grafana/scenes@5.21.2--canary.949.11612383247.0 ```
torkelo commented 2 weeks ago

Currently the expressionBuilder method is unable to reference the application context, and use-cases where application state can influence how a particular ad hoc variable is interpolated is not supported.

this does not make sense, and will break the state flow. As the ad-hoc filter will not update when the other scene state changes.

If you want to link together some other scene state you need to update the ad-hoc filter based on that other scene state, this will make sure the state updates happen (IE some other variable or scene state update =>. update ad-hoc filter that depends on it => re-issue queries)

using state inside a the rendering of filter expression like this breaks the state flow/dependency (meaning this ad-hoc variable will not update when that depend state is updated)

torkelo commented 2 weeks ago

we need to conditionally ignore filters that match the label in the active tab so users can include multiple values by clicking on buttons in the result panels for a single label without needing to modify the ad-hoc variable state

Hm... not sure I understand, there is a ad-hoc filter with a label filter set but you you want to see all possible values like as if this filter was not there? why is it there if it not supposed to have an effect?

gtk-grafana commented 2 weeks ago

Hm... not sure I understand, there is a ad-hoc filter with a label filter set but you you want to see all possible values like as if this filter was not there? why is it there if it not supposed to have an effect?

Basically we want users to be able to add multiple include filters in the viz:

https://github.com/user-attachments/assets/16969171-a758-41ff-a844-6317679c9985

And in the combobox:

https://github.com/user-attachments/assets/5ba5372b-3971-441f-b8e1-3333d2e0a98d

Otherwise adding any include filter that matches the key of the active tab will run the queries and hide other panels that the user might want to add.

But I think I understand why this approach won't work, and realizing it might be better/easier to add a new optional property to the AdHocFilterWithLabels so the application can keep certain filters in the combobox UI, but we can exclude them in the expressionBuilder

e.g.

export interface AdHocFilterWithLabels extends AdHocVariableFilter {
  keyLabel?: string;
  valueLabels?: string[];
  excludeFromQuery?: boolean // new prop that can be used in expressionBuilder, but doesn't impact UI layout
}

What do you think?

gtk-grafana commented 2 weeks ago

@torkelo closing this naive approach, thanks for the feedback! Will shift to approach outlined above (although a bit more generic) in https://github.com/grafana/scenes/pull/951

torkelo commented 2 weeks ago

@gtk-grafana yea I was thinking something like an exclude or filter property.

But what purpose does filters have that are not used in a query?

gtk-grafana commented 2 weeks ago

@torkelo Because they are used in different application contexts. Following the example of the video PoC above: in the "level" tab, I don't want including the "error" filter to exclude info, warn, etc, so I can include more level filters after adding the first one, but when I switch to the namespace tab, I do want those filters applied so I only see namespaces and levels that are in the filters I selected in the previous tab.

torkelo commented 2 weeks ago

but if it's the same adhoc filter variable how can this be filter only be applied to one part of the scene? Feels like you need a locally scoped filter variable that excludes some filters

gtk-grafana commented 2 weeks ago

I was able to get it working locally by updating the filter "meta" on dependent state changes with the proposal on https://github.com/grafana/scenes/pull/951.

Having more locally scoped variables was one of my initial attempts, but the variable I am attempting to convert is already rendered in the top scene and needs URL state to sync across all routes.

gtk-grafana commented 2 weeks ago

I started running into issues with that approach because I'd need to subscribe to the variable and set the meta state on changes within the combobox which trigger duplicate queries.

I'll try creating a "replica" of the "primary" top level variable that is not rendered at a lower scope which can subscribe to the primary and update the filters appropriately.