Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Gladorme wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2f19a36
wip
Gladorme Feb 25, 2026
71e2bb7
[ENHANCEMENT/BREAKINGCHANGE] timneserieschart: add query name support…
Gladorme Feb 26, 2026
59cbfe1
Align with shared
Gladorme Jun 23, 2026
7b238ae
Fix cue
Gladorme Jun 26, 2026
eb2f80f
QueryName -> QueryIndex
Gladorme Jun 26, 2026
2aa0ea3
Backward compatible
Gladorme Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion timeserieschart/schemas/migrate/migrate.cue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ spec: {
// migrate fixedColor overrides to querySettings when applicable
#querySettings: [
for i, target in (*#panel.targets | []) {
queryIndex: i
queryName: "Query #\(i+1)"
for override in (*#panel.fieldConfig.overrides | [])
if (override.matcher.id == "byName" || override.matcher.id == "byRegexp" || override.matcher.id == "byFrameRefID") && override.matcher.options != _|_
for property in override.properties
Expand Down
12 changes: 6 additions & 6 deletions timeserieschart/schemas/migrate/tests/color-based-on-legend-text/expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,28 +7,28 @@
},
"querySettings": [
{
"queryIndex": 0,
"queryName": "Query #1",
"colorMode": "fixed",
"colorValue": "#EAB839"
},
{
"queryIndex": 1,
"queryName": "Query #2",
"colorMode": "fixed",
"colorValue": "#0A437C"
},
{
"queryIndex": 2,
"queryName": "Query #3",
"colorMode": "fixed",
"colorValue": "#890F02",
"areaOpacity": 1
},
{
"queryIndex": 4,
"queryName": "Query #5",
"colorMode": "fixed",
"colorValue": "#6D1F62"
},
{
"queryIndex": 5,
"queryName": "Query #6",
"colorMode": "fixed",
"colorValue": "#052B51"
}
Expand All @@ -47,4 +47,4 @@
"min": 0
}
}
}
}
14 changes: 5 additions & 9 deletions timeserieschart/schemas/migrate/tests/multiple-overrides/expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@
"legend": {
"position": "right",
"mode": "table",
"values": [
"min",
"max",
"mean"
]
"values": ["min", "max", "mean"]
},
"yAxis": {
"format": {
Expand All @@ -25,25 +21,25 @@
},
"querySettings": [
{
"queryIndex": 1,
"queryName": "Query #2",
"colorMode": "fixed",
"colorValue": "#5794F2",
"lineStyle": "dashed",
"areaOpacity": 0
},
{
"queryIndex": 2,
"queryName": "Query #3",
"colorMode": "fixed",
"colorValue": "#F2495C",
"areaOpacity": 0
},
{
"queryIndex": 3,
"queryName": "Query #4",
"colorMode": "fixed",
"colorValue": "#3274D9"
},
{
"queryIndex": 4,
"queryName": "Query #5",
"colorMode": "fixed",
"colorValue": "#fade2a",
"lineStyle": "dashed",
Expand Down
4 changes: 3 additions & 1 deletion timeserieschart/schemas/time-series.cue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
package model

import (
"strings"

"github.com/perses/shared/cue/common"
)

Expand Down Expand Up @@ -60,7 +62,7 @@ spec: close({
}

#querySettings: [...{
queryIndex: int & >=0
queryName: strings.MinRunes(1)
colorMode?: "fixed" | "fixed-single" // NB: "palette" could be added later
colorValue?: =~"^#(?:[0-9a-fA-F]{3}){1,2}$" // hexadecimal color code
lineStyle?: #lineStyle
Comment on lines +65 to 70

@shahrokni shahrokni Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Image

@Gladorme Gladorme Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@shahrokni shahrokni Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@jgbernalp jgbernalp Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the JSON editor is editing directly you can do all sorts of things, including adding empty fields. IMO this is not an issue on the UI but on the backend validation, for optional fields there might be weird things allowed such as this example, but we probably cannot catch all the cases. I'd say we can improve the backend validation and unmarshalling in a different PR, so the resulting structure excludes empty objects if the schema says so.

@Gladorme Gladorme Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Cue, we can't enforce validation on at least one field :/

@Gladorme Gladorme Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it has not impact on the UI if empty object

Expand Down
2 changes: 1 addition & 1 deletion timeserieschart/sdk/go/time-series.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ const (
)

type QuerySettingsItem struct {
QueryIndex uint `json:"queryIndex" yaml:"queryIndex"`
QueryName string `json:"queryName" yaml:"queryName"`
ColorMode ColorMode `json:"colorMode,omitempty" yaml:"colorMode,omitempty"`
ColorValue string `json:"colorValue,omitempty" yaml:"colorValue,omitempty"`
LineStyle string `json:"lineStyle,omitempty" yaml:"lineStyle,omitempty"`
Expand Down
69 changes: 35 additions & 34 deletions timeserieschart/src/QuerySettingsEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import DeleteIcon from 'mdi-material-ui/DeleteOutline';
import AddIcon from 'mdi-material-ui/Plus';
import CloseIcon from 'mdi-material-ui/Close';
import { produce } from 'immer';
import { useDataQueriesContext } from '@perses-dev/plugin-system';
import { generateQueryNames, useDataQueriesContext } from '@perses-dev/plugin-system';
import {
DEFAULT_AREA_OPACITY,
LINE_STYLE_CONFIG,
Expand All @@ -42,7 +42,7 @@ import {
} from './time-series-chart-model';

const DEFAULT_COLOR_VALUE = '#555';
const NO_INDEX_AVAILABLE = -1; // invalid array index value used to represent the fact that no query index is available
const NO_INDEX_AVAILABLE = '-1'; // invalid array index value used to represent the fact that no query index is available

export function QuerySettingsEditor(props: TimeSeriesChartOptionsEditorProps): ReactElement {
const { onChange, value } = props;
Expand All @@ -64,13 +64,13 @@ export function QuerySettingsEditor(props: TimeSeriesChartOptionsEditorProps): R
focusRef.current = false;
}, [querySettingsList?.length]);

const handleQueryIndexChange = (e: React.ChangeEvent<HTMLInputElement>, i: number): void => {
const handleQueryNameChange = (e: React.ChangeEvent<HTMLInputElement>, i: number): void => {
if (querySettingsList !== undefined) {
handleQuerySettingsChange(
produce(querySettingsList, (draft) => {
const querySettings = draft?.[i];
if (querySettings) {
querySettings.queryIndex = parseInt(e.target.value);
querySettings.queryName = e.target.value;
}
})
);
Expand Down Expand Up @@ -224,22 +224,23 @@ export function QuerySettingsEditor(props: TimeSeriesChartOptionsEditorProps): R
};

const { queryDefinitions } = useDataQueriesContext();
const queryCount = queryDefinitions.length;

const queryNames: string[] = useMemo(() => generateQueryNames(queryDefinitions), [queryDefinitions]);

// Compute the list of query indexes for which query settings are not already defined.
// This is to avoid already-booked indexes to still be selectable in the dropdown(s)
const availableQueryIndexes = useMemo(() => {
const bookedQueryIndexes = querySettingsList?.map((querySettings) => querySettings.queryIndex) ?? [];
const allQueryIndexes = Array.from({ length: queryCount }, (_, i) => i);
return allQueryIndexes.filter((_, queryIndex) => !bookedQueryIndexes.includes(queryIndex));
}, [querySettingsList, queryCount]);
const availableQueryNames = useMemo(() => {
return queryNames.filter((name) => {
return !querySettingsList?.some((qs) => qs.queryName === name);
});
}, [queryNames, querySettingsList]);

const firstAvailableQueryIndex = useMemo(() => {
return availableQueryIndexes[0] ?? NO_INDEX_AVAILABLE;
}, [availableQueryIndexes]);
const firstAvailableQueryName = useMemo(() => {
return availableQueryNames[0] ?? NO_INDEX_AVAILABLE;
}, [availableQueryNames]);

const defaultQuerySettings: QuerySettingsOptions = {
queryIndex: firstAvailableQueryIndex,
queryName: firstAvailableQueryName,
};

const addQuerySettingsInput = (): void => {
Expand All @@ -257,20 +258,19 @@ export function QuerySettingsEditor(props: TimeSeriesChartOptionsEditorProps): R

return (
<Stack>
{queryCount === 0 ? (
{queryDefinitions.length === 0 ? (
<Typography mb={2} fontStyle="italic">
No query defined
</Typography>
) : (
querySettingsList?.length &&
querySettingsList.map((querySettings, i) => (
querySettingsList?.map((querySettings, i) => (
<QuerySettingsInput
inputRef={i === querySettingsList.length - 1 ? recentlyAddedInputRef : undefined}
key={i}
querySettings={querySettings}
availableQueryIndexes={availableQueryIndexes}
onQueryIndexChange={(e) => {
handleQueryIndexChange(e, i);
availableQueryNames={availableQueryNames}
onQueryNameChange={(e) => {
handleQueryNameChange(e, i);
}}
onColorModeChange={(e) => handleColorModeChange(e, i)}
onColorValueChange={(color) => handleColorValueChange(color, i)}
Expand All @@ -291,7 +291,7 @@ export function QuerySettingsEditor(props: TimeSeriesChartOptionsEditorProps): R
/>
))
)}
{queryCount > 0 && firstAvailableQueryIndex !== NO_INDEX_AVAILABLE && (
{queryDefinitions.length > 0 && firstAvailableQueryName !== NO_INDEX_AVAILABLE && (
<Button variant="contained" startIcon={<AddIcon />} sx={{ marginTop: 1 }} onClick={addQuerySettingsInput}>
Add Query Settings
</Button>
Expand All @@ -302,8 +302,8 @@ export function QuerySettingsEditor(props: TimeSeriesChartOptionsEditorProps): R

interface QuerySettingsInputProps {
querySettings: QuerySettingsOptions;
availableQueryIndexes: number[];
onQueryIndexChange: (e: React.ChangeEvent<HTMLInputElement>) => void;
availableQueryNames: string[];
onQueryNameChange: (e: React.ChangeEvent<HTMLInputElement>) => void;
onColorModeChange: (e: React.ChangeEvent<HTMLInputElement>) => void;
onColorValueChange: (colorValue: string) => void;
onLineStyleChange: (lineStyle: string) => void;
Expand All @@ -323,9 +323,9 @@ interface QuerySettingsInputProps {
}

function QuerySettingsInput({
querySettings: { queryIndex, colorMode, colorValue, lineStyle, areaOpacity, format },
availableQueryIndexes,
onQueryIndexChange,
querySettings: { queryName, colorMode, colorValue, lineStyle, areaOpacity, format },
availableQueryNames,
onQueryNameChange,
onColorModeChange,
onColorValueChange,
onLineStyleChange,
Expand All @@ -343,7 +343,7 @@ function QuerySettingsInput({
onFormatChange,
}: QuerySettingsInputProps): ReactElement {
// current query index should also be selectable
const selectableQueryIndexes = availableQueryIndexes.concat(queryIndex).sort((a, b) => a - b);
const selectableQueryName = availableQueryNames.sort((a, b) => a.localeCompare(b));

// State for dropdown menu
const [anchorEl, setAnchorEl] = useState<null | HTMLElement>(null);
Expand Down Expand Up @@ -394,14 +394,15 @@ function QuerySettingsInput({
<TextField
select
inputRef={inputRef}
value={queryIndex}
value={queryName}
label="Query"
onChange={onQueryIndexChange}
onChange={onQueryNameChange}
sx={{ minWidth: '75px' }}
>
{selectableQueryIndexes.map((qi) => (
<MenuItem value={queryName}>{queryName}</MenuItem>
{selectableQueryName.map((qi) => (
<MenuItem key={`query-${qi}`} value={qi}>
#{qi + 1}
{qi}
</MenuItem>
))}
</TextField>
Expand All @@ -414,7 +415,7 @@ function QuerySettingsInput({
<MenuItem value="fixed">Fixed</MenuItem>
</TextField>
<OptionsColorPicker
label={`Query n°${queryIndex + 1}`}
label={queryName}
color={colorValue || DEFAULT_COLOR_VALUE}
onColorChange={onColorValueChange}
/>
Expand Down Expand Up @@ -449,7 +450,7 @@ function QuerySettingsInput({
{/* Area Opacity section */}
{areaOpacity !== undefined && (
<SettingsSection label="Opacity" onRemove={onRemoveAreaOpacity}>
{/* Spacer as I don't want to add a prop to SettingsSection for left-padding just for that case.. */}
{/* Spacer as I don't want to add a prop to SettingsSection for left-padding just for that case. */}
<Box />
<Slider
value={areaOpacity}
Expand Down Expand Up @@ -506,7 +507,7 @@ function QuerySettingsInput({
</Stack>
{/* Delete Button for this query settings */}
<Box sx={{ display: 'flex', alignItems: 'center', justifyContent: 'center' }}>
<IconButton aria-label={`delete settings for query n°${queryIndex + 1}`} onClick={onDelete}>
<IconButton aria-label={`delete settings for query '${queryName}'`} onClick={onDelete}>
<DeleteIcon />
</IconButton>
</Box>
Expand Down
12 changes: 10 additions & 2 deletions timeserieschart/src/TimeSeriesChartPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
legendValues,
getCalculations,
CalculationType,
defaultQueryName,
} from '@perses-dev/plugin-system';
import {
ChartInstance,
Expand Down Expand Up @@ -199,12 +200,19 @@ export function TimeSeriesChartPanel(props: TimeSeriesChartProps): ReactElement
// TODO: Look into performance optimizations and moving parts of mapping to the lower level chart
for (let queryIndex = 0; queryIndex < queryResults.length; queryIndex++) {
const result = queryResults[queryIndex];
if (result === undefined) {
console.warn(
'Something went wrong with the query result mapping, result is undefined for query index',
queryIndex
);
continue;
}

// Retrieve querySettings for this query, if exists.
// queries & querySettings indices do not necessarily match, so we have to check the tail value of the $ref attribute
let querySettings: QuerySettingsOptions | undefined;
for (const item of querySettingsList ?? []) {
if (item.queryIndex === queryIndex) {
if (item.queryName === result.definition.spec.name || item.queryName === defaultQueryName(queryIndex)) {
querySettings = item;
// We don't break the loop here just in case there are multiple querySettings defined for the
// same queryIndex, because in that case we want the last one to take precedence.
Expand All @@ -230,7 +238,7 @@ export function TimeSeriesChartPanel(props: TimeSeriesChartProps): ReactElement
seriesName: formattedSeriesName,
seriesIndex,
querySettings: querySettings,
queryHasMultipleResults: (queryResults[queryIndex]?.data?.series?.length ?? 0) > 1,
queryHasMultipleResults: (result.data?.series?.length ?? 0) > 1,
});

// We add a unique id for the chart to disambiguate items across charts
Expand Down
2 changes: 1 addition & 1 deletion timeserieschart/src/time-series-chart-model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export interface TimeSeriesChartOptions {
}

export interface QuerySettingsOptions {
queryIndex: number;
queryName: string;
colorMode?: 'fixed' | 'fixed-single';
colorValue?: string;
lineStyle?: LineStyleType;
Comment on lines 42 to 50

@shahrokni shahrokni Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Image

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 Gladorme Jun 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
9 changes: 5 additions & 4 deletions timeserieschart/src/utils/palette-gen.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import { defaultQueryName } from '@perses-dev/plugin-system';
import { TimeSeriesChartVisualOptions } from '../time-series-chart-model';
import { getSeriesColor, getAutoPaletteColor, getCategoricalPaletteColor, SeriesColorProps } from './palette-gen';

Expand Down Expand Up @@ -119,7 +120,7 @@ describe('getSeriesColor', () => {
seriesName: testSeriesName,
seriesIndex: 0,
querySettings: {
queryIndex: 0,
queryName: defaultQueryName(0),
colorMode: 'fixed',
colorValue: '#000',
},
Expand All @@ -142,7 +143,7 @@ describe('getSeriesColor', () => {
seriesName: testSeriesName,
seriesIndex: 0,
querySettings: {
queryIndex: 0,
queryName: defaultQueryName(0),
colorMode: 'fixed',
colorValue: '#000',
},
Expand All @@ -165,7 +166,7 @@ describe('getSeriesColor', () => {
seriesName: testSeriesName,
seriesIndex: 0,
querySettings: {
queryIndex: 0,
queryName: defaultQueryName(0),

@Nexucis Nexucis Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Gladorme Gladorme Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Gladorme Gladorme Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jgbernalp Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

colorMode: 'fixed-single',
colorValue: '#000',
},
Expand All @@ -188,7 +189,7 @@ describe('getSeriesColor', () => {
seriesName: testSeriesName,
seriesIndex: 0,
querySettings: {
queryIndex: 0,
queryName: defaultQueryName(0),
colorMode: 'fixed-single',
colorValue: '#000',
},
Expand Down
Loading