[ENHANCEMENT] TimeSeriesChart: add query name support for query settings

[Gladorme]

Description

Depends on: perses/shared#69

Add support for query name in query settings + deprecating queryIndex.

Migration guide:

• Via the UI:
  Go the dashboard, edit timeserieschart panel, go to query settings, update any value and save (it will migrate all query settings of the panel to queryName)

• Manually:
  Replace queryIndex by queryName.
  Replace number by string Query #<index + 1> or use helper defaultQueryName.
  Example: queryIndex: 0 become queryName: 'Query #1'.

Screenshots

Queries:

Query settings:

Checklist

• Pull request has a descriptive title and context useful to a reviewer.
• Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the
  following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, DOC,IGNORE.
• All commits have DCO signoffs.

UI Changes

• Changes that impact the UI include screenshots and/or screencasts of the relevant changes.
• Code follows the UI guidelines.

[Nexucis]

that's a massive breaking change. This will break every dashboard created without dashboard as code or by migrating from Grafana.

[Gladorme]

Yeah, I will check if I can keep old data model

[Gladorme]

Ok, I remember why I did this change. We can't keep index, because it will cause issue when re-ordering queries with query settings. I will rename queryName to queryIndex, but result will be the same => they will need to update queryIndex.

[jgbernalp]

not sure why we are dropping the queryIndex, the index could be 0 and the name could be whatever. If name is a new field for better UX, queryIndex can be kept as it is.

[shahrokni]

It seems that the idea behind this condition was that, if queryIndex was undefined then queryName would already exist. So, simply return the current one. However, this could go wrong. The QuerySettingsOptions object could be completely empty due to the fact that all fields are optional. Therefore, regardless of how queryName is generated, it is still technically possible to receive an empty object. This may open the door to bugs in future.

/* ALL FIELDS ARE OPTIONAL */
export interface QuerySettingsOptions {
  queryName?: string;
  /**
   * @deprecated Use `queryName` instead.
   */
  queryIndex?: number;
  colorMode?: 'fixed' | 'fixed-single';
  colorValue?: string;
  lineStyle?: LineStyleType;
  areaOpacity?: number;
  format?: FormatOptions;
}

So, maybe?

if (querySettings.queryIndex === undefined && querySettings.queryName!==undefined)

[Gladorme]

Unfortunately, we can't enforce validation on cue on one field if one is missing and we don't want to do breaking change

[shahrokni]

I understand that from the backend perspective a none-optional queryName could be problematic and probably a breaking change.

queryName?: strings.MinRunes(1)

However, from the UI perspective this field looks mandatory and there is no way to drop it. This means that from UI we could still keep it as a none-optional field.

The fact that we could have empty object query settings is not a good idea in my opinion, although from the UI a query name is always guaranteed.

export interface QuerySettingsOptions {
  queryName?: string;
  /**
   * @deprecated Use `queryName` instead.
   */
  queryIndex?: number;
  colorMode?: 'fixed' | 'fixed-single';
  colorValue?: string;
  lineStyle?: LineStyleType;
  areaOpacity?: number;
  format?: FormatOptions;
}

[Gladorme]

Unfortunately, we can't enforce validation on cue on one field if one is missing and we don't want to do breaking change

[shahrokni]

Look at the following payload that I crafted and sent to the backend.
Since the schema is fully optional now, such a payload is persisted. I think this should be avoided. If we insist that we should keep all fields optional, then maybe we should intercept the req and drop the querySettings completely. (I am just discussing the payload point of view)

[Gladorme]

Users could do it yes, but UI form enforce query name. So ok to me with limitation we have

[shahrokni]

Users could do it yes, but UI form enforce query name. So ok to me with limitation we have

@jgbernalp Could we also have your opinion about this?