fix: clamp over-wide widgets, drop zero-width widgets, and properly handle invalid order by

pkg/types/dashboardtypes/perses_v1_to_v2_layouts.go

@@ -180,6 +180,11 @@ func placedWidgetIDs(data StorableDashboardData) map[string]bool {
 		for _, e := range layout {
 			if m, ok := e.(map[string]any); ok {
 				if i, ok := m["i"].(string); ok && i != "" {
+					// A zero-width placement doesn't render in the v1 UI; treat it as
+					// unplaced so the widget is dropped rather than migrated invisibly.
+					if w, ok := m["w"].(float64); ok && w <= 0 {
+						continue
+					}
 					ids[i] = true
 				}
 			}
@@ -320,6 +325,9 @@ func compactGridItemsVertically(items []dashboard.GridItem) {
 		if l.X < 0 {
 			l.X = 0
 		}
+		if l.X+l.Width > gridColumnCount { // still overflows (wider than the grid) → clamp width so x+width = cols
+			l.Width = gridColumnCount - l.X
+		}
 		if l.Y < 0 {
 			l.Y = 0
 		}

pkg/types/dashboardtypes/perses_v1_to_v2_queries_malformed.go

@@ -3,6 +3,7 @@ package dashboardtypes
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"log/slog"
 	"regexp"
 	"strings"
@@ -108,26 +109,12 @@ func normalizeFunctionArgs(query map[string]any) {
 	}
 }
 
-// malformedOrderByValueKeys are v4 order-by columnNames meaning "order by the aggregation value"
-// that the v5 aggregation validator rejects (validateOrderByForAggregation). All resolve
-// to the same aggregation key. Add more as they surface. The frontend passes these
-// through (the query-service resolves them), but the v2 dashboard validator only accepts
-// a real aggregation key.
-var malformedOrderByValueKeys = map[string]bool{
-	"#SIGNOZ_VALUE":                  true,
-	"A":                              true,
-	"A.count()":                      true,
-	"__result":                       true,
-	"value":                          true,
-	"A.p99(duration_nano)":           true,
-	"aws_Kafka_MessagesInPerSec_max": true,
-	"byte_in_count":                  true,
-	"(http_server_request_duration_ms.bucket)": true,
-}
-
-// normalizeOrderByKeys rewrites any orderBy columnName in orderByValueKeys to the
-// v5-valid aggregation key. Left untouched if the key can't resolve (no aggregation to
-// name).
+// normalizeOrderByKeys rewrites any orderBy columnName the v5 aggregation validator
+// would reject (validateOrderByForAggregation) to the canonical aggregation value key.
+// v1 tolerated free-form "order by the value" aliases (#SIGNOZ_VALUE, the query name,
+// the raw metric/expression) that the query-service resolved at query time; v2 accepts
+// only a real order key. Anything already valid (a group-by key, an aggregation
+// expression/alias/index) is left alone. No-op if no aggregation key can be named.
 func normalizeOrderByKeys(query map[string]any) {
 	orders, ok := query["orderBy"].([]any)
 	if !ok {
@@ -137,17 +124,88 @@ func normalizeOrderByKeys(query map[string]any) {
 	if !ok {
 		return
 	}
+	valid := validAggregationOrderKeys(query)
 	for _, o := range orders {
 		order, ok := o.(map[string]any)
 		if !ok {
 			continue
 		}
-		if cn, _ := order["columnName"].(string); malformedOrderByValueKeys[cn] {
+		if cn, _ := order["columnName"].(string); cn != "" && !valid[cn] {
 			order["columnName"] = key
 		}
 	}
 }
 
+// validAggregationOrderKeys is a line-for-line mirror of validateOrderByForAggregation
+// (querybuildertypesv5/validation.go) over the untyped query map instead of a typed
+// QueryBuilderQuery. Keep the two in lockstep — every insertion here must match one
+// there — so a side-by-side read makes any drift obvious. The only adaptations: fields
+// are read out of maps, the type switch on the aggregation becomes a switch on the
+// query signal (metrics vs logs/traces), and a not-yet-upgraded group-by still carries
+// the v4 "key" instead of the v5 "name".
+func validAggregationOrderKeys(query map[string]any) map[string]bool {
+	validOrderKeys := make(map[string]bool)
+
+	for _, gb := range asObjects(query["groupBy"]) {
+		name, _ := gb["name"].(string)
+		if name == "" {
+			name, _ = gb["key"].(string)
+		}
+		validOrderKeys[name] = true
+	}
+
+	signal := signalFromDataSource(query["dataSource"])
+	for i, agg := range asObjects(query["aggregations"]) {
+		validOrderKeys[fmt.Sprintf("%d", i)] = true
+
+		switch signal {
+		// TraceAggregation / LogAggregation (identical bodies in the validator).
+		case telemetrytypes.SignalTraces, telemetrytypes.SignalLogs:
+			if alias, _ := agg["alias"].(string); alias != "" {
+				validOrderKeys[alias] = true
+			}
+			expression, _ := agg["expression"].(string)
+			validOrderKeys[expression] = true
+
+		// MetricAggregation.
+		case telemetrytypes.SignalMetrics:
+			// Also allow the generic __result pattern
+			validOrderKeys["__result"] = true
+
+			metricName, _ := agg["metricName"].(string)
+			spaceRaw, _ := agg["spaceAggregation"].(string)
+			timeRaw, _ := agg["timeAggregation"].(string)
+			spaceAggregation := metrictypes.SpaceAggregation{String: valuer.NewString(spaceRaw)}
+			timeAggregation := metrictypes.TimeAggregation{String: valuer.NewString(timeRaw)}
+
+			validOrderKeys[fmt.Sprintf("%s(%s)", spaceAggregation.StringValue(), metricName)] = true
+			if timeAggregation != metrictypes.TimeAggregationUnspecified {
+				validOrderKeys[fmt.Sprintf("%s(%s)", timeAggregation.StringValue(), metricName)] = true
+			}
+			if timeAggregation != metrictypes.TimeAggregationUnspecified && spaceAggregation != metrictypes.SpaceAggregationUnspecified {
+				validOrderKeys[fmt.Sprintf("%s(%s(%s))", spaceAggregation.StringValue(), timeAggregation.StringValue(), metricName)] = true
+			}
+		}
+	}
+
+	return validOrderKeys
+}
+
+// asObjects returns the map elements of a []any, skipping non-object entries.
+func asObjects(raw any) []map[string]any {
+	items, ok := raw.([]any)
+	if !ok {
+		return nil
+	}
+	out := make([]map[string]any, 0, len(items))
+	for _, it := range items {
+		if m, ok := it.(map[string]any); ok {
+			out = append(out, m)
+		}
+	}
+	return out
+}
+
 // aggregationOrderKey names the first aggregation the way validateOrderByForAggregation
 // expects: "space(metricName)" for metrics, the expression for logs/traces.
 func aggregationOrderKey(query map[string]any) (string, bool) {

pkg/types/dashboardtypes/perses_v1_to_v2_test.go

@@ -1738,6 +1738,47 @@ func TestConvertV1WidgetQueryRewritesValueOrderKeyAfterDefaultAggregation(t *tes
 	assert.Equal(t, "count()", spec.Order[0].Key.Name, "#SIGNOZ_VALUE resolves to the injected default aggregation")
 }
 
+// A metric query ordering by a value alias the v5 validator rejects (here the raw
+// metric name, never enumerated anywhere) is rewritten to the canonical aggregation
+// key, while a genuine group-by order key is left alone. Guards the allowlist
+// approach: validity is derived from the query, not a hardcoded set of bad keys.
+func TestConvertV1WidgetQueryRewritesUnknownMetricValueOrderKey(t *testing.T) {
+	widget := map[string]any{
+		"id":         "m-1",
+		"panelTypes": "graph",
+		"query": map[string]any{
+			"queryType": "builder",
+			"builder": map[string]any{
+				"queryData": []any{
+					map[string]any{
+						"queryName":    "A",
+						"expression":   "A",
+						"dataSource":   "metrics",
+						"aggregations": []any{map[string]any{"metricName": "http_requests_total", "spaceAggregation": "sum"}},
+						"groupBy":      []any{map[string]any{"key": "service.name", "dataType": "string", "type": "resource"}},
+						"orderBy": []any{
+							map[string]any{"columnName": "http_requests_total", "order": "desc"},
+							map[string]any{"columnName": "service.name", "order": "asc"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	queries := (&v1Decoder{}).convertV1WidgetQuery(widget, PanelKindTimeSeries)
+	require.Len(t, queries, 1)
+
+	wrapper, ok := queries[0].Spec.Plugin.Spec.(*BuilderQuerySpec)
+	require.True(t, ok)
+	spec, ok := wrapper.Spec.(qb.QueryBuilderQuery[qb.MetricAggregation])
+	require.True(t, ok, "metrics query should dispatch to MetricAggregation, got %T", wrapper.Spec)
+
+	require.Len(t, spec.Order, 2)
+	assert.Equal(t, "sum(http_requests_total)", spec.Order[0].Key.Name, "unknown value alias rewritten to the aggregation key")
+	assert.Equal(t, "service.name", spec.Order[1].Key.Name, "valid group-by order key left alone")
+}
+
 func TestConvertV1WidgetQueryInjectsCountForNoopOnAggregationPanel(t *testing.T) {
 	// A logs query with the list-style "noop" operator placed on an aggregation
 	// panel (graph). createAggregationsShapeSafe drops noop, leaving no aggregation;
@@ -2084,6 +2125,26 @@ func TestConvertV1LayoutsClampsXBounds(t *testing.T) {
 	assert.Equal(t, 6, grid.Items[1].X) // x+w=16>12 shifted left to 12-6
 }
 
+func TestConvertV1LayoutsClampsOverwideWidth(t *testing.T) {
+	// A widget wider than the 12-col grid (e.g. carried over from a 24-col v1 grid)
+	// can't be shifted to fit, so its width is clamped so x+width = grid width —
+	// otherwise it overflows and v2 validation rejects it.
+	data := StorableDashboardData{
+		"widgets": []any{map[string]any{"id": "wide", "panelTypes": "graph", "query": singleLogsBuilderQuery()}},
+		"layout":  []any{map[string]any{"i": "wide", "x": float64(0), "y": float64(0), "w": float64(24), "h": float64(6)}},
+	}
+
+	d := &v1Decoder{}
+	layouts := d.convertV1Layouts(data, d.convertV1Panels(data["widgets"]))
+	require.NoError(t, d.errIfHasMalformedFields())
+	require.Len(t, layouts, 1)
+	grid, ok := layouts[0].Spec.(*dashboard.GridLayoutSpec)
+	require.True(t, ok)
+	require.Len(t, grid.Items, 1)
+	assert.Equal(t, 0, grid.Items[0].X)
+	assert.Equal(t, 12, grid.Items[0].Width) // w=24 clamped to 12 so x+width = 12
+}
+
 // TestConvertV1LayoutsToleratesNonObjectPanelMap covers templates that store
 // panelMap as {rowID: []widgetID} instead of the canonical {rowID: {widgets,
 // collapsed}}. The frontend reads such an entry as "not collapsed" (it accesses
@@ -2152,6 +2213,33 @@ func TestConvertV1LayoutsDropsEntryForUnrenderableWidget(t *testing.T) {
 	assert.Equal(t, "#/spec/panels/p-1", spec.Items[0].Content.Ref)
 }
 
+func TestRetainPlacedWidgetsDropsZeroWidth(t *testing.T) {
+	// z-1 is placed with zero width, which the v1 UI doesn't render. It must be
+	// dropped entirely: no panel, and no dangling layout entry.
+	data := StorableDashboardData{
+		"widgets": []any{
+			map[string]any{"id": "p-1", "panelTypes": "graph", "query": singleLogsBuilderQuery()},
+			map[string]any{"id": "z-1", "panelTypes": "graph", "query": singleLogsBuilderQuery()},
+		},
+		"layout": []any{
+			map[string]any{"i": "p-1", "x": float64(0), "y": float64(0), "w": float64(6), "h": float64(6)},
+			map[string]any{"i": "z-1", "x": float64(6), "y": float64(0), "w": float64(0), "h": float64(6)},
+		},
+	}
+
+	d := &v1Decoder{}
+	panels := d.convertV1Panels(retainPlacedWidgets(data))
+	require.Contains(t, panels, "p-1")
+	require.NotContains(t, panels, "z-1", "a zero-width widget is not retained → no panel")
+
+	layouts := d.convertV1Layouts(data, panels)
+	require.Len(t, layouts, 1)
+	spec, ok := layouts[0].Spec.(*dashboard.GridLayoutSpec)
+	require.True(t, ok)
+	require.Len(t, spec.Items, 1, "the zero-width widget's layout entry is dropped too")
+	assert.Equal(t, "#/spec/panels/p-1", spec.Items[0].Content.Ref)
+}
+
 func TestConvertV1LayoutsDropsCollapsedChildWithNoPanel(t *testing.T) {
 	// A collapsed section lists a child ("ghost") that has no widget at all — a
 	// deleted widget still referenced in panelMap. It produces no panel and no

pkg/types/querybuildertypes/querybuildertypesv5/formula.go

@@ -140,49 +140,6 @@ type seriesLookup struct {
 	// maps a variable to its series keys, letting evaluation iterate a single
 	// variable's series directly.
 	variableToSeriesKeys map[string][]string
-	// seriesKey -> label set encoded as id pairs, used as the "subset" side of isSubset
-	// during dedup and evaluation so label comparison is an integer compare.
-	encodedLabels map[string][]encodedLabel
-}
-
-// encodedLabel is a label's key name and value replaced with stable integer ids by an
-// encodedLabelsBuilder, so comparing two encodedLabels is an integer compare rather than
-// a per-byte string compare (runtime.efaceeq/strequal/memeqbody).
-type encodedLabel struct {
-	keyID   uint32
-	valueID uint32
-}
-
-// encodedLabelsBuilder stores a stable uint32 id for each distinct label key name and label
-// value (dictionary encoding). Populated single-threaded in buildSeriesLookup and
-// read-only thereafter, so the parallel evaluation phase never writes to it.
-type encodedLabelsBuilder struct {
-	encodedKeyIDs   map[string]uint32
-	encodedValueIDs map[any]uint32
-}
-
-func newEncodedLabelsBuilder() *encodedLabelsBuilder {
-	return &encodedLabelsBuilder{encodedKeyIDs: map[string]uint32{}, encodedValueIDs: map[any]uint32{}}
-}
-
-func (b *encodedLabelsBuilder) encode(labels []*Label) []encodedLabel {
-	out := make([]encodedLabel, len(labels))
-	for i, label := range labels {
-		encodedKey, ok := b.encodedKeyIDs[label.Key.Name]
-		if !ok {
-			encodedKey = uint32(len(b.encodedKeyIDs)) // acts as a sequential counter.
-			b.encodedKeyIDs[label.Key.Name] = encodedKey
-		}
-
-		encodedValue, ok := b.encodedValueIDs[label.Value]
-		if !ok {
-			encodedValue = uint32(len(b.encodedValueIDs)) // acts as a sequential counter.
-			b.encodedValueIDs[label.Value] = encodedValue
-		}
-
-		out[i] = encodedLabel{keyID: encodedKey, valueID: encodedValue}
-	}
-	return out
 }
 
 // FormulaEvaluator handles formula evaluation b/w time series from different aggregations
@@ -342,7 +299,7 @@ func (fe *FormulaEvaluator) EvaluateFormula(timeSeriesData map[string]*TimeSerie
 	// Work per label-set is cheap  enough that spawning a goroutine per item
 	// costs more in scheduler signaling  than it saves in parallelism.
 	const numWorkers = 4
-	workCh := make(chan labelSetCandidate, len(uniqueLabelSets))
+	workCh := make(chan []*Label, len(uniqueLabelSets))
 	resultChan := make(chan *TimeSeries, len(uniqueLabelSets))
 
 	var wg sync.WaitGroup
@@ -391,12 +348,8 @@ func (fe *FormulaEvaluator) buildSeriesLookup(timeSeriesData map[string]*TimeSer
 		seriesMetadata: make(map[string]*TimeSeries),
 
 		variableToSeriesKeys: make(map[string][]string),
-
-		encodedLabels: make(map[string][]encodedLabel),
 	}
 
-	encodedLabelsBuilder := newEncodedLabelsBuilder()
-
 	for variable, aggRef := range fe.aggRefs {
 		// We are only interested in the time series data for the queries that are
 		// involved in the formula expression.
@@ -446,7 +399,6 @@ func (fe *FormulaEvaluator) buildSeriesLookup(timeSeriesData map[string]*TimeSer
 			if _, exists := lookup.data[seriesKey]; !exists {
 				lookup.data[seriesKey] = make(map[int64]float64, len(series.Values))
 				lookup.seriesMetadata[seriesKey] = series
-				lookup.encodedLabels[seriesKey] = encodedLabelsBuilder.encode(series.Labels)
 				lookup.variableToSeriesKeys[variable] = append(lookup.variableToSeriesKeys[variable], seriesKey)
 			}
 
@@ -509,74 +461,58 @@ func (fe *FormulaEvaluator) buildSeriesKey(variable string, seriesIndex int, lab
 // The result of any expression that uses the series with `{"service": "frontend", "operation": "GET /api"}`
 // and `{"service": "frontend"}` would be the series with `{"service": "frontend", "operation": "GET /api"}`
 // So, we create a set of labels sets that can be termed as candidates for the final result.
-func (fe *FormulaEvaluator) findUniqueLabelSets(lookup *seriesLookup) []labelSetCandidate {
-	// original labels (for the result series) paired with their encoded form (for the
-	// subset comparison).
-	type labelSet struct {
-		labels  []*Label
-		encoded []encodedLabel
-	}
-	allLabelSets := make([]labelSet, 0, len(lookup.seriesMetadata))
-	for key, series := range lookup.seriesMetadata {
-		allLabelSets = append(allLabelSets, labelSet{labels: series.Labels, encoded: lookup.encodedLabels[key]})
+func (fe *FormulaEvaluator) findUniqueLabelSets(lookup *seriesLookup) [][]*Label {
+	var allLabelSets [][]*Label
+
+	// Collect all label sets from series metadata
+	for _, series := range lookup.seriesMetadata {
+		allLabelSets = append(allLabelSets, series.Labels)
 	}
 
-	// sort the label sets by the number of labels in descending order.
-	slices.SortFunc(allLabelSets, func(i, j labelSet) int {
-		if len(i.labels) > len(j.labels) {
+	// sort the label sets by the number of labels in descending order
+	slices.SortFunc(allLabelSets, func(i, j []*Label) int {
+		if len(i) > len(j) {
 			return -1
 		}
-		if len(i.labels) < len(j.labels) {
+		if len(i) < len(j) {
 			return 1
 		}
 		return 0
 	})
 
-	// Find unique label sets using integer-id label comparison.
-	var uniqueSets []labelSetCandidate
+	// Find unique label sets using proper label comparison
+	var uniqueSets [][]*Label
+	var uniqueMaps []map[string]any
 	for _, labelSet := range allLabelSets {
 		isUnique := true
-		for _, uniqueSet := range uniqueSets {
-			if isSubset(uniqueSet.encodedKeyToEncodedValueMap, labelSet.encoded) {
+		for _, uniqueMap := range uniqueMaps {
+			if isSubset(uniqueMap, labelSet) {
 				isUnique = false
 				break
 			}
 		}
 		if isUnique {
-			uniqueSets = append(uniqueSets, labelSetCandidate{
-				labels:                      labelSet.labels,
-				encodedKeyToEncodedValueMap: encodedLabelsToMap(labelSet.encoded),
-			})
+			uniqueSets = append(uniqueSets, labelSet)
+			uniqueMaps = append(uniqueMaps, labelsToMap(labelSet))
 		}
 	}
 
 	return uniqueSets
 }
 
-// labelSetCandidate is a maximal label set kept by findUniqueLabelSets: the original
-// labels (preserved for the result series) plus a keyID->valueID map used as the
-// "superset" when matching series during evaluation.
-type labelSetCandidate struct {
-	labels                      []*Label
-	encodedKeyToEncodedValueMap map[uint32]uint32
-}
-
-func encodedLabelsToMap(labels []encodedLabel) map[uint32]uint32 {
-	m := make(map[uint32]uint32, len(labels))
+func labelsToMap(labels []*Label) map[string]any {
+	m := make(map[string]any, len(labels))
 	for _, label := range labels {
-		m[label.keyID] = label.valueID
+		m[label.Key.Name] = label.Value
 	}
 	return m
 }
 
 // isSubset reports whether every label in subset is present with the same value in
 // supersetMap (i.e. subset ⊆ superset).
-func isSubset(supersetMap map[uint32]uint32, subset []encodedLabel) bool {
+func isSubset(supersetMap map[string]any, subset []*Label) bool {
 	for _, label := range subset {
-		// each key and value of string/any type in the overall label set in the whole result
-		// has a corresponding unique uint32 assigned to it by buildSeriesLookup, which helps us
-		// do a simple integer comparison in place of a much more expensive str/any comparison.
-		if valID, ok := supersetMap[label.keyID]; !ok || valID != label.valueID {
+		if val, ok := supersetMap[label.Key.Name]; !ok || val != label.Value {
 			return false
 		}
 	}
@@ -584,7 +520,7 @@ func isSubset(supersetMap map[uint32]uint32, subset []encodedLabel) bool {
 }
 
 // evaluateForLabelSet performs formula evaluation for a specific label set.
-func (fe *FormulaEvaluator) evaluateForLabelSet(targetLabels labelSetCandidate, lookup *seriesLookup) *TimeSeries {
+func (fe *FormulaEvaluator) evaluateForLabelSet(targetLabels []*Label, lookup *seriesLookup) *TimeSeries {
 	// Find matching series for each variable
 	variableData := make(map[string]map[int64]float64)
 	// not every series would have a value for every timestamp
@@ -592,14 +528,14 @@ func (fe *FormulaEvaluator) evaluateForLabelSet(targetLabels labelSetCandidate,
 	// for the variable
 	var allTimestamps = make(map[int64]struct{})
 
-	// target.encodedKeyToEncodedValueMap is the superset map, precomputed once in
-	// findUniqueLabelSets and reused across every series comparison below.
-	targetMap := targetLabels.encodedKeyToEncodedValueMap
+	// targetLabels is fixed for this call, so build its lookup once and reuse it
+	// across every series comparison below.
+	targetMap := labelsToMap(targetLabels)
 
 	for variable := range fe.aggRefs {
 		// only this variable's series.
 		for _, seriesKey := range lookup.variableToSeriesKeys[variable] {
-			if isSubset(targetMap, lookup.encodedLabels[seriesKey]) {
+			if isSubset(targetMap, lookup.seriesMetadata[seriesKey].Labels) {
 				if timestampData, exists := lookup.data[seriesKey]; exists {
 					variableData[variable] = timestampData
 					// Collect all timestamps
@@ -687,8 +623,8 @@ func (fe *FormulaEvaluator) evaluateForLabelSet(targetLabels labelSetCandidate,
 	}
 
 	// Preserve original label structure and metadata
-	resultLabels := make([]*Label, len(targetLabels.labels))
-	copy(resultLabels, targetLabels.labels)
+	resultLabels := make([]*Label, len(targetLabels))
+	copy(resultLabels, targetLabels)
 
 	return &TimeSeries{
 		Labels: resultLabels,