GroupByVariable: Sync label to URL #705

bfmatei · 2024-04-23T11:09:17Z

This PR syncs labels of the values in the group by variable similarly to what we introduced for ad-hoc filters.

Unlike ad-hoc filters, group by is a little bit more of a special case hence the reason why some of the changes below. The solution might not be optimal, so please feel free to suggested changes and improvements.

Changes:

introduced custom URL sync handler
- it works similarly to the ad-hoc filters one
- this is needed as currently we use the one multi-value variable one and we don't want this changed for all variables
the state of group by variable received a new prop: urlOptions where we store the options that we received via the URL
- when fetching the options, we concat the urlOptions with what we receive from the datasource (or with the defaultOptions)
- this is needed because of validateAndUpdate and also because changeValueTo is sometimes called without the texts array
we leverage the fact that the value of the MultiSelect can be a SelectableValue hence we can enforce the labels on values
moved some common functions to variables utils

The gotcha: let's assume we pass a value via URL. This value is not returned by the datasource hence we consider it a custom value. We then remove the custom value using the UI button. When opening the dropdown again, the custom value can be found in the dropdown. Is this a behavior we want to keep? You can also see this behavior in the demo below.

PS: tests will come in a subsequent push. For the time being I'm trying to decide upon edge cases.

Demo:

Screen.Recording.2024-04-23.at.14.07.53.mov

📦 Published PR as canary version: 4.20.0--canary.705.9031064372.0

✨ Test out this PR locally via:

npm install @grafana/scenes@4.20.0--canary.705.9031064372.0
# or 
yarn add @grafana/scenes@4.20.0--canary.705.9031064372.0

torkelo · 2024-04-23T11:30:28Z

packages/scenes/src/variables/groupby/GroupByVariable.tsx

  public getValueOptions(args: VariableGetOptionsArgs): Observable<VariableValueOption[]> {
+    const urlOptions = this.state.urlOptions ?? [];
+


confused, thought group by variables are lazy so we don't call this function unless when the dropdown opens. So why do we need URL options here?

#687

There is also validateAndUpdate which calls getValueOptions.

Sometimes changeValueTo is called without texts and this.state.options is used to determine the value of texts. Check this out: https://github.com/grafana/scenes/blob/main/packages/scenes/src/variables/variants/MultiValueVariable.ts#L257

but how does they relate to url sync?

I'm confused by this as well, we never pass available options via URL.

Sorry, I should have given a more in-depth explanation for this.

So, the URL values are technically custom values until the options are loaded. After the options are loaded:

some of these values are normal values (they can be found in the received options)

some of these values are traditional custom values (the label is the same as the value and they are not found in the received options)

and some of these values are a special case of custom values (the label is different from the value and they are not found in the received options).

urlOptions was needed because of the last type of values as there are situations where we can lose the label of a custom value. For example, when calling changeValueTo method without a texts parameter, the text is extracted from the available options. In order to overcome this, we can either require the texts parameter (which is a huge breaking change) or make the custom value/label pairs available to it.

In the case of GroupByVariable this can be easily solved because changeValueTo is called only from the renderer but for other types of MultiValueVariable this gets tricky to solve.

A neat side-effect of the urlOptions was that the 3rd type of custom values could also live as an independent option. For example, we receive a custom value with a label via the URL: value is MyValue and label is MyLabel. This custom value does not exist in the options that we load from the API. The user removes it by clicking the "X" button but realizes it should have stayed there. When opening the menu, the user could simply pick MyLabel from the list. Because they only saw MyLabel, they didn't know what was the actual value of it. Indeed, this is an edge case tho'.

I'll push an update to the PR in a couple of minutes with an implementation w/o urlOptions.

torkelo · 2024-04-23T11:34:47Z

packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts

+      this._sceneObject.setState({
+        urlOptions: values.map((value, idx) => ({
+          value,
+          text: labels[idx],
+        })),
+      });


don't really understand why we need this

We need the urlOptions in order not to lose the label of a custom value when we select an item from the available options.

Having them as MetricFindValue as opposed to a map or even SelectableValue directly makes it easier to reuse it without additional processing in the actual variable.

We need the urlOptions in order not to lose the label of a custom value when we select an item from the available options.

not sure I follow, what do you mean "loose the label of a custom value" ? a custom value can never have different value/label

Provided more details here: #705 (comment)

packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts

torkelo · 2024-04-23T11:37:12Z

packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts

+import { GroupByVariable } from './GroupByVariable';
+import { toUrlCommaDelimitedString } from '../utils';
+
+export class GroupByVariableUrlSyncHandler implements SceneObjectUrlSyncHandler {


Maybe we can reuse MultiValueUrlSyncHandler but with an option to sync label? since they are so close to identical

Yup, I can merge them into one. I'll implement it in a subsequent commit.

I started working on this but to be honest it kinda makes it weird to bring urlOptions to multi-value variable. WDYT?

@bfmatei I think the whole concept of urlOptions is strange, need to figure out a way we can avoid it.

the URL state should only be concerned with setting the current value/text not the options

Pushing an update without it now.

torkelo · 2024-05-10T05:55:00Z

packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts

+      }
+
+      urlValue = Array.isArray(urlValue) ? urlValue : [urlValue];
+      const valuesLabelsPairs = urlValue.map((value) => (value ? value.split(',') : [value]));


Would like to change this so that we implement it for all variables with value/label (ie in default MultiValueVariable), and only sync label when value != label.

I am worried though, this could be a breaking change, all current saved URLs with variable values with commas would break? (I assume new URL values we escape commas, but not existing saved links)

Besides breaking the URLs, taking care of the texts is a problem that will need to be solved as well. I described it in the long comment above (i.e. require the texts parameter for changeValueTo method.

grafanabot · 2024-05-13T12:30:11Z

🚀 PR was released in v4.20.0 🚀

dprokop

@bfmatei it looks and works good!

I've noticed a minor difference in the filters and group by rendering.

Let's assume both filter and group by use the same dimensions set:

label1, v1
label2, v2
label3, v3

Then let's assume an old url was preserved that did not contain label in the url. So we have url like this:

?var-Filter=%20v1%7C%3D%7Caaa&var-Group%20by=%20v1

Now, unless the select was opened, what's synced into the renderer in both filter and group by is v1. But then when openening the Group by v1 is replaced with label1. Which is fine IMO. But it doesn't work like this for filters - the correct dimension is selected, but the value of the select does not change to label1 when opened. See the video:

Screen.Recording.2024-05-13.at.14.26.05.mov

Not a deal breaker, but something that we can improve in the followup PR.

bfmatei added minor Increment the minor version when merged release Create a release when this pr is merged area/variables Issues related to variables system in scenes labels Apr 23, 2024

bfmatei requested review from torkelo, dprokop and ashharrison90 April 23, 2024 11:09

bfmatei self-assigned this Apr 23, 2024

torkelo reviewed Apr 23, 2024

View reviewed changes

bfmatei marked this pull request as ready for review April 26, 2024 11:37

torkelo reviewed May 10, 2024

View reviewed changes

bfmatei added 6 commits May 10, 2024 13:34

GroupByVariable: Sync label to URL

f3ac536

Unescape delimiters

d22490a

Fix typechecking

906b6a0

Fix typechecking

e284000

Add tests

d2b74a9

Remove urlOptions

91edd05

bfmatei force-pushed the bogdan/groupby-label-url branch from 97eba21 to 91edd05 Compare May 10, 2024 10:37

bfmatei requested a review from torkelo May 10, 2024 10:37

Fix typechecking

c861f37

torkelo approved these changes May 13, 2024

View reviewed changes

bfmatei merged commit 81ebbde into main May 13, 2024
3 checks passed

bfmatei deleted the bogdan/groupby-label-url branch May 13, 2024 12:27

grafanabot added the released label May 13, 2024

dprokop approved these changes May 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GroupByVariable: Sync label to URL #705

GroupByVariable: Sync label to URL #705

bfmatei commented Apr 23, 2024 •

edited by grafanabot

torkelo Apr 23, 2024

bfmatei Apr 23, 2024

torkelo May 10, 2024

dprokop May 10, 2024

bfmatei May 10, 2024

torkelo Apr 23, 2024 •

edited

bfmatei Apr 23, 2024

torkelo May 10, 2024

bfmatei May 10, 2024

torkelo Apr 23, 2024

bfmatei Apr 23, 2024

bfmatei Apr 26, 2024

torkelo May 10, 2024

bfmatei May 10, 2024

torkelo May 10, 2024

bfmatei May 10, 2024

grafanabot commented May 13, 2024

dprokop left a comment •

edited

		public getValueOptions(args: VariableGetOptionsArgs): Observable<VariableValueOption[]> {
		const urlOptions = this.state.urlOptions ?? [];

GroupByVariable: Sync label to URL #705

GroupByVariable: Sync label to URL #705

Conversation

bfmatei commented Apr 23, 2024 • edited by grafanabot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torkelo Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafanabot commented May 13, 2024

dprokop left a comment • edited

Choose a reason for hiding this comment

bfmatei commented Apr 23, 2024 •

edited by grafanabot

torkelo Apr 23, 2024 •

edited

dprokop left a comment •

edited