feat: hide the < and > operators for filters with string values

[joannaWebDev]

Description

This PR hides the < and > operators for filters with string values, to ensure that users are not presented with irrelevant numeric operators for non-numeric filter values.

Changes

• _getTagKeys handles setting the areTheFilterValuesNumbers prop to true when the values are numeric
• Added tests to validate

Relates to db-o11y issue

[dprokop]

I'm not convinced we should accept this. As much as I like the idea and I think it's the right thing to do to provide operators that are relevant, I'd rather like to solve the problem upstream on the ad hoc filters API level, so that the information about the type of the values for a filter is provided through the response of the getTagValues. Thoughts @torkelo?

[torkelo]

so that the information about the type of the values for a filter is provided through the response of the getTagValues

Is that not a bit late? you set values after the tag key and operator so by that point it's too late

feels like it should be prop on the key maybe?

[joannaWebDev]

so that the information about the type of the values for a filter is provided through the response of the getTagValues

Is that not a bit late? you set values after the tag key and operator so by that point it's too late

feels like it should be prop on the key maybe?

I think the _getKeys is only responsible of the filter keys, returning something like this

{text:` 'schema', value: 'schema'}

Where as the _getValuesFor, indeed is being executed after the _getKeys, but is the API responsible for fetching the actual values corresponding to the above keys, meaning this should be the place to access the typeof.

{
    "key": "schema",
    "values": [
        {
            "text": "public",
            "value": "public" // this is where we know the type
        },
        {
            "text": "private",
            "value": "private"
        },
    ]
}

[dprokop]

Is that not a bit late? you set values after the tag key and operator so by that point it's too late

yes sorry, mistaken getTagKeys with getTagValues.

[dprokop]

Where as the _getValuesFor, indeed is being executed after the _getKeys, but is the API responsible for fetching the actual values corresponding to the above keys, meaning this should be the place to access the typeof.

@joannaWebDev i had getTagKeysin mind, thich returns the dimensions available, sorry my mistake. Each dimension should have a type property available indicating what kind of value it represents, driven by the datasource. With that, I think we should figure out how to implement that upstream in the DataSourceApi.getTagKeys:

/**
 * Get tag keys for adhoc filters
 */
getTagKeys?(options?: DataSourceGetTagKeysOptions<TQuery>): Promise<GetTagResponse> | Promise<MetricFindValue[]>;

[joannaWebDev]

Where as the _getValuesFor, indeed is being executed after the _getKeys, but is the API responsible for fetching the actual values corresponding to the above keys, meaning this should be the place to access the typeof.

@joannaWebDev i had getTagKeysin mind, thich returns the dimensions available, sorry my mistake. Each dimension should have a type property available indicating what kind of value it represents, driven by the datasource. With that, I think we should figure out how to implement that upstream in the DataSourceApi.getTagKeys:

/**
 * Get tag keys for adhoc filters
 */
getTagKeys?(options?: DataSourceGetTagKeysOptions<TQuery>): Promise<GetTagResponse> | Promise<MetricFindValue[]>;

Ah, I see. We have the values: GetTagResponse which expose the value?: string | number;
Changed

[svennergr]

FWIW, Explore Logs just overwrites _getOperators and hides some operators like that. Could also be a viable way for other apps, maybe?

[joannaWebDev]

FWIW, Explore Logs just overwrites _getOperators and hides some operators like that. Could also be a viable way for other apps, maybe?

Thank you for pointing that out! Overwriting _getOperators as Explore Logs could be a viable approach. However, my intention here is slightly different. Instead of relying on overrides at the app level, my proposal aims to define and apply the relevant operators directly at the root level. This way, we ensure that the operators are consistent and logical across different use cases from the start, reducing the need for app-specific customizations.

So if the values are strings, we should not be seeing < or >.
And if we disallow custom values, we should not be seeing the regex operators either (previous PR) 😉

[gtk-grafana]

Also want to note that while this PR doesn't impact Explore Logs because we overwrite the _getOperators implementation, Loki does have numeric comparisons for "strings" like duration > 10s or bytes <= 100MB. Additionally there are some int/numeric fields like userId which we do not want numeric operations used.

[joannaWebDev]

Also want to note that while this PR doesn't impact Explore Logs because we overwrite the _getOperators implementation, Loki does have numeric comparisons for "strings" like duration > 10s or bytes <= 100MB. Additionally there are some int/numeric fields like userId which we do not want numeric operations used.

This is very important feedback, thank you for bringing this up.
The way I see it, in order to avoid breaking the logic across apps, we need to have this additional areTheFilterValuesNumbers prop, but we would need to set its state directly in the component that uses the AdHocFiltersVariable, which in the db-o11y case, would look smx like this:

export function makeAdHocFilters(onFilterAdd?: () => void): AdHocFiltersVariable {
  return new AdHocFiltersVariable({


    getTagKeysProvider: async (set: AdHocFiltersVariable) => {
      try {
        const ds = await getDataSourceSrv().get(set.state.datasource);

        **// Check if the keys are numeric
        const areKeysNumeric = Array.isArray(response)
          ? response.every((key) => !isNaN(Number(key.value)))
          : false;

        // Update the state with the numeric information
        set.setState({ areTheFilterValuesNumbers: areKeysNumeric });**

        const filteredKeys = response.filter((key) =>
          set.state.filters.every((f) => f.key !== key.value)
        );

        onFilterAdd?.();

        return { values: DEFAULT_KEYS.concat(filteredKeys), replace: true };
      } catch (error) {
        return { values: [] };
      }
    },

    getTagValuesProvider: async (set: AdHocFiltersVariable, filter: AdHocVariableFilter) => {  },
  });
}

wdyt? @dprokop @gtk-grafana

[gtk-grafana]

@joannaWebDev Take a look at #1034 for more information about the Explore Logs use-case, I'd like to be able to set metadata from the getTagKeysProvider api response to decide which operators are used for a specific key within an ad-hoc variable.
That PR doesn't attempt to solve the generic case which you are addressing here, but I would prefer allowing the developers to define which operators to use in the response of the getTagKeysProvider

export type getTagKeysProvider = (
  variable: AdHocFiltersVariable,
  currentKey: string | null,
  operators?: OperatorDefinition[]
) => Promise<{ replace?: boolean; values: GetTagResponse | MetricFindValue[] }>;

And then if operators are returned by the getTagKeysProvider, we would overwrite the default OPERATORS.

My argument is that the operations are very specific to each datasource, or even variable types within a datasource, and instead of trying to solve the problem for everyone, we should make it easier for developers to customize the operators used depending on application context.

Additionally, (although probably irrelevant to this discussion) in Explore Logs we use ad-hoc variables for Loki line-filters, which doesn't use any of the "default" operators, but instead uses |=, |~ etc, so ad-hoc variables are flexible enough to be used with data that doesn't match the assumptions in this PR (although we implemented a custom renderer).

But you probably want to check with @dprokop and ignore everything I said, my perspective is quite narrow, and we're doing some funky stuff with variables to get the functionality we need to build Explore Logs

[joannaWebDev]

@joannaWebDev Take a look at #1034 for more information about the Explore Logs use-case, I'd like to be able to set metadata from the getTagKeysProvider api response to decide which operators are used for a specific key within an ad-hoc variable. That PR doesn't attempt to solve the generic case which you are addressing here, but I would prefer allowing the developers to define which operators to use in the response of the getTagKeysProvider

export type getTagKeysProvider = (
  variable: AdHocFiltersVariable,
  currentKey: string | null,
  operators?: OperatorDefinition[]
) => Promise<{ replace?: boolean; values: GetTagResponse | MetricFindValue[] }>;

And then if operators are returned by the getTagKeysProvider, we would overwrite the default OPERATORS.

My argument is that the operations are very specific to each datasource, or even variable types within a datasource, and instead of trying to solve the problem for everyone, we should make it easier for developers to customize the operators used depending on application context.

Additionally, (although probably irrelevant to this discussion) in Explore Logs we use ad-hoc variables for Loki line-filters, which doesn't use any of the "default" operators, but instead uses |=, |~ etc, so ad-hoc variables are flexible enough to be used with data that doesn't match the assumptions in this PR (although we implemented a custom renderer).

But you probably want to check with @dprokop and ignore everything I said, my perspective is quite narrow, and we're doing some funky stuff with variables to get the functionality we need to build Explore Logs

Exposing the operators looks like the cleanest approach to me. wdyt @dprokop?

[gtk-grafana]

Actually, there might be a solution in core Grafana for the getTagKeys method to optionally return operators, which would give the datasource control over what key gets what operator, which should solve this more generically:

// change getTagKeys to return a new `MetricFindKeys` interface
getTagKeys?(options?: DataSourceGetTagKeysOptions): Promise<MetricFindKeys[]>;

export interface MetricFindKeys {
  text: string;
  value?: string | number;
  group?: string;
  expandable?: boolean;

  // New optional prop to set the default operators for this key
  operators?: SelectableValue[]
}

I can try to find some time to mock up a PoC later this week or next, but I'm curious if this would work for you @joannaWebDev?

[joannaWebDev]

@gtk-grafana That should work, yes. 🙏🏻

[mdvictor]

From what I'm seeing, and I might lack a bunch of info especially on the prometheus side of things, I think having another params in getTagKeys might not be helpful? I reckon the prometheus API just returns a list of keys that aren't helpful in deciding what operators to use - we'd also have to somehow verify each and every value for a given key in our backend and see if it's an actual string of characters, or a parsable number, or some other accepted comparison value (e.g.: 10s), which sounds very bad taking into accounts that some value calls last quite a bit and return large lists of values. And I guess this might be the problem with other datasources too? But again, I might be missing some important information here.

I'm thinking maybe a better way, although probably not the best, would be to allow adhocs to also have an operator provider similar to the other values and keys providers that would override the default operators with whatever the app dev needs. Then it could just be used in specific scenarios where we know that comparison is not allowed, for example. cc @gtk-grafana @joannaWebDev

[gtk-grafana]

@mdvictor for Loki when getting tag keys we call the detected_fields endpoint that returns a "type" with each key which is associated with specific operations, my proposal above was the simplest solution I could think of that works for Loki, it might not necessarily be appropriate for other datasources.

But for the Loki use case, by the time the proposed operator provider would be called we've already gotten and thrown away the type we need to pick the correct operator in the getTagKeys.

The other problem is where do we store the "type" on an ad-hoc variable so if a user comes back to an existing ad-hoc filter and edits the value/operator, we won't call the getTagKeys provider again, so your proposal works better for this case, but would still require an additional API call.

All in all I'm not opposed to having the option for an operator provider, but on its own I don't think it solves the problem generically for all datasources...

Proposal:

Scene apps need the ability to add arbitrary metadata to ad-hoc filters at any point in the adhoc filter lifecycle (i.e. before or after it has an operator/value). This metadata would be saved in the URL state for the ad-hoc variable filter.

That way Loki datasources can add the "type" (and parser (and JSON path)) metadata to a filter when a key is added using the response of the getTagKeys provider, then we don't need to re-query the tagKeys provider in a later session or if a user shares a url and the user wants to edit an operator for a filter.

Similarly Prometheus or other datasources that might not have operator metadata returned by the getTagKeys provider could add meta to a _wip filter in the proposed getOperatorsProvider, or other meta could be added when getting the values, or at any other time when dealing with ad-hoc filters.