Variables: support object-based values with multiple properties

[grafakus]

Based on #1228, resolves https://github.com/grafana/grafana-org/issues/555

Design doc

This PR:

• Introduces options providers, with 3 built-in providers implemented: csv, json and query
• Enables runtime registration of new options providers, for extensibility
• Enables the json and query providers to support object values with multiple properties
• Refactors CustomVariable and QueryVariable to use options providers
• Refactors MultiValueVariable to support value objects
• Adds a new demo for object values

The other changes are consequences of the changes above.

Todos

• DataSourceVariable to use its own options provider?

[torkelo]

hm.. this is tricky, torn on how to proceed, feels messy somehow to add it like this.

maybe a unified variable approach is best, at least from a technical stand point, interested in trying it out just to see how it would change things, simplify things

torn a bit where the text/value prop bit should be as well, inside the value / options provider abstraction or outside it

[torkelo]

not sure I like this, think it would be better if this was a simple interface.

something like

export interface VariableValueProvider {
  getValues(arg: VariableGetOptionsArgs): Observable<MetricFindValueWithOptionalProperties>
}

Hm.. Interested in trying to do this in a new SelectVariable (and leave CustomVariable and QueryVariable as is for now). just to get a sense of how that would work/look.

The query string state property of CustomVariable should not really exist, it's an option that should be part of provider object/class (as it's type and value depends on the provider).

[torkelo]

did a quick test here: 1449343

hm.. the problem with that approach is that it becomes a bit of a pain to update the state of the value providers (have to create a new one), would have to make the value providers scene objects to solve that, which feels a bit overkill / hoping to avoid that :(

[grafakus]

We can inject an instance of the provider, that would be neat. My thought here is to try keeping the changes minimal from a consumer perspective and not to break the existing contract (like passing query):

new CustomVariable({ query: '[]', optionsProvider: { type: 'json' } });

vs

new CustomVariable({ optionsProvider: { getOptions(arg) { ... } } });

Interested in trying to do this in a new SelectVariable (and leave CustomVariable and QueryVariable as is for now). just to get a sense of how that would work/look.

👍🏾👍🏾 It would give us a good idea of how proposal 3 of the design doc would look like.

[torkelo]

If we are to make a change to CustomVariable and still use the query property for the csv/json content, I think we should try to make the smallest / simplest thing, just adding a new option.

valuesFormat: 'csv'  | 'json' 

I think we can wait with textProp and valueProp for now (trying to simplify, remove all options that are not 100% needed). we can use same heurstic as toMetricFindValues (property named value and text, if not use first string for value, rest goes to properties)

[grafakus]

About the heuristic to find values and labels, wouldn't it be better not to assume we will be able to rely on them (or at that it'll be easy to rely on them) for all the datasources that will start getting support (Elastic and Postgres) and for all the scenarios from a user perspective?

[dprokop]

+1 on @torkelo comment of having minimal interface change here, type: 'csv' | 'json', or valuesFormat: 'csv' | 'json'. With custom variable it's really valuesFormat?: 'json' as CSV is the default already.

[torkelo]

Sorry for backtracking a bit on previous feedback, variables are complex, it takes time to find the right change, if we are to modify the current Custom and Query I think a more minimalistic change could work

thoughts?

[torkelo]

This feels unnecessary here, as it's inside the QueryVariable, we know the provider

[grafakus]

It was added it to be able to pass custom valueProp and textProp

[torkelo]

this looks less then ideal

[grafakus]

On the pros side, it's quite simple to rely on the factory function ; on the cons side, a new instance has to be built every time.

To mitigate the cons, could we rely on memoization?

[torkelo]

Both elastic and postgres use the metricFindQuery so already return MetricFindValue, so this function will return already on line 29 (unless we update those data sources to handle variables differently)

[grafakus]

I think it was mentioned when we reached the consensus on the design doc. Besides pinging the owners of the Elastic and Postgres datasources, is the change in this PR enough?

[torkelo]

If we are to make a change to CustomVariable and still use the query property for the csv/json content, I think we should try to make the smallest / simplest thing, just adding a new option.

valuesFormat: 'csv'  | 'json' 

I think we can wait with textProp and valueProp for now (trying to simplify, remove all options that are not 100% needed). we can use same heurstic as toMetricFindValues (property named value and text, if not use first string for value, rest goes to properties)

[dprokop]

In my opinion this PR introduces a lot of unjustified infrastructure(providers, runtime registration, factories) for really solving the problem of supporting json values. The provider approach would be nice if we were to scale the format options, but I cannot see introducing any new formats any time soon - maybe yaml would make sense, but I doubt truly.

Things like buildOptionsProvider(... could really be solved by instance functions (this.parseXXX) that under the hood could use utils for the parsing if we want to abstract anything. So much easier to reason about IMO, think about debugging for example: CustomVariable.parseValues() vs CustomVariable/buildOptionsProvider()/registry/provider/getOptions

i think solution like this in the variable should be enough to get the same result:

private parseValues(query: string, format?: 'csv' | 'json') {
  return format === 'json' ? this.parseJson(query) : this.parseCsv(query);
}

[dprokop]

Do we anticipate more formats introduction? I don't see a justification for having this complexity at this stage. Eventually when we decide to unify custom/query variable under List variable, this will become unnecessary.

[dprokop]

+1 on @torkelo comment of having minimal interface change here, type: 'csv' | 'json', or valuesFormat: 'csv' | 'json'. With custom variable it's really valuesFormat?: 'json' as CSV is the default already.