@cryptotaxi247 / netdata-1 / commits / 9a4b3dc98

chartengine: fix validated framework issues (#22138)

Ilya Mashchenko committed Apr 4, 2026 at 14:43 UTC 9a4b3dc9820c3482076157624d2950a10ffc3cea
26 files changed +791 -213
src/go/plugin/framework/chartengine/attempt.go
-1
@@ -178,7 +178,6 @@ func prepareAndCommitPlan(engine *Engine, reader metrix.Reader) (Plan, error) {
178 if err != nil {
179 return Plan{}, err
180 }
181 - defer attempt.Abort()
181
182 plan := attempt.Plan()
183 if err := attempt.Commit(); err != nil {
src/go/plugin/framework/chartengine/autogen.go
+1 -1
@@ -658,7 +658,7 @@ func getAutogenCounterUnits(metric string) string {
658 }
659
660 func getAutogenSummaryUnits(metric string) string {
661 - return getAutogenCounterUnits(metric)
661 + return getAutogenGaugeUnits(metric)
662 }
663
664 func getAutogenMetricUnits(metric string) string {
src/go/plugin/framework/chartengine/autogen_test.go
+5 -2
@@ -140,6 +140,7 @@ func runTestBuildSummaryQuantileAutogenRoute(t *testing.T) {
140 labels map[string]string
141 wantID string
142 wantDim string
143 + wantUnits string
144 }{
145 "summary quantile excludes quantile label and keeps absolute algorithm": {
146 metricName: "svc.request_duration_seconds",
@@ -148,8 +149,9 @@ func runTestBuildSummaryQuantileAutogenRoute(t *testing.T) {
149 "method": "GET",
150 "quantile": "0.99",
151 },
151 - wantID: "svc.request_duration_seconds-instance=db1-method=GET",
152 - wantDim: "quantile_0.99",
152 + wantID: "svc.request_duration_seconds-instance=db1-method=GET",
153 + wantDim: "quantile_0.99",
154 + wantUnits: "seconds",
155 },
156 }
157
@@ -166,6 +168,7 @@ func runTestBuildSummaryQuantileAutogenRoute(t *testing.T) {
168
169 assert.Equal(t, tc.wantID, route.chartID)
170 assert.Equal(t, tc.wantDim, route.dimensionName)
171 + assert.Equal(t, tc.wantUnits, route.units)
172 assert.Equal(t, metrix.SummaryQuantileLabel, route.dimensionKeyLabel)
173 assert.Equal(t, program.AlgorithmAbsolute, route.algorithm)
174 assert.False(t, route.staticDimension)
src/go/plugin/framework/chartengine/compiler.go
+8 -2
@@ -5,6 +5,7 @@ package chartengine
5 import (
6 "fmt"
7 "sort"
8 + "strconv"
9 "strings"
10
11 "github.com/netdata/netdata/go/plugins/pkg/metrix"
@@ -13,7 +14,12 @@ import (
14 "github.com/netdata/netdata/go/plugins/plugin/framework/charttpl"
15 )
16
16 -// Compile converts a validated chart template spec into immutable chartengine IR.
17 +// Compile converts a decoded/default-applied chart template spec into immutable
18 +// chartengine IR.
19 +//
20 +// Callers should prefer charttpl.DecodeYAML, which applies chart_defaults
21 +// inheritance before validation. Compile validates the provided spec but does
22 +// not apply charttpl defaults or mutate the input.
23 func Compile(spec *charttpl.Spec, revision uint64) (*program.Program, error) {
24 if spec == nil {
25 return nil, fmt.Errorf("chartengine: nil template spec")
@@ -433,7 +439,7 @@ func pathIndexes(path []int) string {
439 if i > 0 {
440 b.WriteByte('.')
441 }
436 - b.WriteString(fmt.Sprintf("%d", idx))
442 + b.WriteString(strconv.Itoa(idx))
443 }
444 return b.String()
445 }
src/go/plugin/framework/chartengine/compiler_test.go
+34
@@ -167,6 +167,40 @@ func TestCompileScenarios(t *testing.T) {
167 assert.Equal(t, Priority+321, charts[0].Meta.Priority)
168 },
169 },
170 + "does not apply chart_defaults inheritance during compile": {
171 + spec: charttpl.Spec{
172 + Version: charttpl.VersionV1,
173 + Groups: []charttpl.Group{
174 + {
175 + Family: "Service",
176 + Metrics: []string{"svc_requests_total"},
177 + ChartDefaults: &charttpl.ChartDefaults{
178 + LabelPromoted: []string{"cluster"},
179 + Instances: &charttpl.Instances{
180 + ByLabels: []string{"instance"},
181 + },
182 + },
183 + Charts: []charttpl.Chart{
184 + {
185 + Title: "Requests",
186 + Context: "requests",
187 + Units: "requests/s",
188 + Dimensions: []charttpl.Dimension{
189 + {Selector: "svc_requests_total", Name: "total"},
190 + },
191 + },
192 + },
193 + },
194 + },
195 + },
196 + assert: func(t *testing.T, p *program.Program) {
197 + t.Helper()
198 + charts := p.Charts()
199 + require.Len(t, charts, 1)
200 + assert.Empty(t, charts[0].Labels.PromoteKeys)
201 + assert.Empty(t, charts[0].Identity.InstanceByLabels)
202 + },
203 + },
204 "keeps default chart expiry when lifecycle is present without expire_after_cycles": {
205 spec: charttpl.Spec{
206 Version: charttpl.VersionV1,
src/go/plugin/framework/chartengine/engine.go
-9
@@ -107,15 +107,6 @@ func (e *Engine) ResetMaterialized() {
107 e.mu.Unlock()
108 }
109
110 -// loadYAMLFile reads chart-template YAML from file, compiles and publishes it.
111 -func (e *Engine) loadYAMLFile(path string, revision uint64) error {
112 - spec, err := charttpl.DecodeYAMLFile(path)
113 - if err != nil {
114 - return err
115 - }
116 - return e.Load(spec, revision)
117 -}
118 -
110 // program returns the latest compiled immutable program snapshot.
111 func (e *Engine) program() *program.Program {
112 if e == nil {
src/go/plugin/framework/chartengine/identity.go
+22 -42
@@ -119,41 +119,27 @@ func resolveInstanceLabelValues(identity program.ChartIdentity, labels labelAcce
119 return nil, true, nil
120 }
121
122 - excludeSet := make(map[string]struct{})
123 - seenKeys := make(map[string]struct{})
124 - keys := make([]string, 0, len(identity.InstanceByLabels))
125 - includeAll := false
126 - for _, token := range identity.InstanceByLabels {
127 - switch {
128 - case token.Exclude:
129 - if token.Key != "" {
130 - excludeSet[token.Key] = struct{}{}
131 - }
132 - case token.IncludeAll:
133 - includeAll = true
134 - case token.Key != "":
135 - if _, excluded := excludeSet[token.Key]; excluded {
136 - continue
137 - }
138 - if _, exists := seenKeys[token.Key]; exists {
139 - continue
140 - }
141 - if _, ok := labels.Get(token.Key); !ok {
142 - // Explicit instance key is required to materialize one instance.
143 - return nil, false, nil
144 - }
145 - seenKeys[token.Key] = struct{}{}
146 - keys = append(keys, token.Key)
122 + plan := compileInstanceLabelPlan(identity)
123 + out := make([]instanceLabelValue, 0, len(plan.explicitKeys))
124 + for _, key := range plan.explicitKeys {
125 + value, ok := labels.Get(key)
126 + if !ok {
127 + // Explicit instance key is required to materialize one instance.
128 + return nil, false, nil
129 }
130 + out = append(out, instanceLabelValue{
131 + Key: key,
132 + Value: value,
133 + })
134 }
135
150 - if includeAll {
136 + if plan.includeAll {
137 all := make([]string, 0)
138 labels.Range(func(key, _ string) bool {
153 - if _, excluded := excludeSet[key]; excluded {
139 + if _, excluded := plan.excludeSet[key]; excluded {
140 return true
141 }
156 - if _, exists := seenKeys[key]; exists {
142 + if _, exists := plan.explicitSet[key]; exists {
143 return true
144 }
145 all = append(all, key)
@@ -161,21 +147,15 @@ func resolveInstanceLabelValues(identity program.ChartIdentity, labels labelAcce
147 })
148 sort.Strings(all)
149 for _, key := range all {
164 - seenKeys[key] = struct{}{}
165 - keys = append(keys, key)
166 - }
167 - }
168 -
169 - out := make([]instanceLabelValue, 0, len(keys))
170 - for _, key := range keys {
171 - value, ok := labels.Get(key)
172 - if !ok {
173 - return nil, false, nil
150 + value, ok := labels.Get(key)
151 + if !ok {
152 + return nil, false, nil
153 + }
154 + out = append(out, instanceLabelValue{
155 + Key: key,
156 + Value: value,
157 + })
158 }
175 - out = append(out, instanceLabelValue{
176 - Key: key,
177 - Value: value,
178 - })
159 }
160 return out, true, nil
161 }
src/go/plugin/framework/chartengine/identity_test.go
+34
@@ -176,3 +176,37 @@ func TestRenderChartInstanceIDSanitizesLegacyLabelChars(t *testing.T) {
176 })
177 }
178 }
179 +
180 +func TestRenderChartInstanceIDExcludeWinsRegardlessOfTokenOrder(t *testing.T) {
181 + tests := map[string]struct {
182 + selectors []program.InstanceLabelSelector
183 + wantID string
184 + }{
185 + "exclude after explicit": {
186 + selectors: []program.InstanceLabelSelector{
187 + {Key: "host"},
188 + {Exclude: true, Key: "host"},
189 + },
190 + wantID: "mysql_queries",
191 + },
192 + "exclude before explicit": {
193 + selectors: []program.InstanceLabelSelector{
194 + {Exclude: true, Key: "host"},
195 + {Key: "host"},
196 + },
197 + wantID: "mysql_queries",
198 + },
199 + }
200 +
201 + for name, tc := range tests {
202 + t.Run(name, func(t *testing.T) {
203 + got, ok, err := renderChartInstanceID(program.ChartIdentity{
204 + IDTemplate: program.Template{Raw: "mysql_queries"},
205 + InstanceByLabels: tc.selectors,
206 + }, map[string]string{"host": "db1"})
207 + require.NoError(t, err)
208 + assert.True(t, ok)
209 + assert.Equal(t, tc.wantID, got)
210 + })
211 + }
212 +}
src/go/plugin/framework/chartengine/internal/cache/eviction.go
+1
@@ -13,5 +13,6 @@ func retainSeenEntries[T any](
13 }
14 kept = append(kept, bucket[i])
15 }
16 + clear(bucket[len(kept):])
17 return kept
18 }
src/go/plugin/framework/chartengine/internal/cache/route_cache.go
+23 -8
@@ -73,6 +73,9 @@ func (c *RouteCache[T]) MarkSeenIfPresent(identity metrix.SeriesIdentity, buildS
73 }
74
75 func (c *RouteCache[T]) Store(identity metrix.SeriesIdentity, revision uint64, buildSeq uint64, values []T) {
76 + if !c.beginBuild(buildSeq) {
77 + return
78 + }
79 bucket := c.buckets[identity.Hash64]
80 for i := range bucket {
81 if bucket[i].identity.ID != identity.ID {
@@ -80,7 +83,10 @@ func (c *RouteCache[T]) Store(identity metrix.SeriesIdentity, revision uint64, b
83 }
84 bucket[i].revision = revision
85 bucket[i].values = cloneSlice(values)
83 - c.markSeen(&bucket[i], buildSeq)
86 + if bucket[i].lastSeenBuild != buildSeq {
87 + bucket[i].lastSeenBuild = buildSeq
88 + c.seenCount++
89 + }
90 c.buckets[identity.Hash64] = bucket
91 return
92 }
@@ -93,10 +99,6 @@ func (c *RouteCache[T]) Store(identity metrix.SeriesIdentity, revision uint64, b
99 }
100 c.buckets[identity.Hash64] = append(bucket, entry)
101 c.entryCount++
96 - if c.seenBuild != buildSeq {
97 - c.seenBuild = buildSeq
98 - c.seenCount = 0
99 - }
102 c.seenCount++
103 }
104
@@ -109,6 +111,9 @@ func (c *RouteCache[T]) RetainSeen(buildSeq uint64) RetainSeenStats {
111 if c.entryCount == 0 {
112 return stats
113 }
114 + if buildSeq < c.seenBuild {
115 + return stats
116 + }
117 if c.seenBuild != buildSeq {
118 // No cached entries were observed in this build; drop all.
119 clear(c.buckets)
@@ -143,9 +148,8 @@ func (c *RouteCache[T]) RetainSeen(buildSeq uint64) RetainSeenStats {
148 }
149
150 func (c *RouteCache[T]) markSeen(entry *routeCacheEntry[T], buildSeq uint64) {
146 - if c.seenBuild != buildSeq {
147 - c.seenBuild = buildSeq
148 - c.seenCount = 0
151 + if !c.beginBuild(buildSeq) {
152 + return
153 }
154 if entry.lastSeenBuild == buildSeq {
155 return
@@ -154,6 +158,17 @@ func (c *RouteCache[T]) markSeen(entry *routeCacheEntry[T], buildSeq uint64) {
158 c.seenCount++
159 }
160
161 +func (c *RouteCache[T]) beginBuild(buildSeq uint64) bool {
162 + if buildSeq < c.seenBuild {
163 + return false
164 + }
165 + if c.seenBuild != buildSeq {
166 + c.seenBuild = buildSeq
167 + c.seenCount = 0
168 + }
169 + return true
170 +}
171 +
172 func cloneSlice[T any](in []T) []T {
173 if len(in) == 0 {
174 return nil
src/go/plugin/framework/chartengine/internal/cache/route_cache_test.go
+59
@@ -106,6 +106,65 @@ func TestRouteCache(t *testing.T) {
106 assert.False(t, ok)
107 },
108 },
109 + "stale build sequence is ignored for store and retain": {
110 + run: func(t *testing.T) {
111 + rc := NewRouteCache[string]()
112 + a := metrix.SeriesIdentity{ID: "a", Hash64: 30}
113 + b := metrix.SeriesIdentity{ID: "b", Hash64: 31}
114 + c := metrix.SeriesIdentity{ID: "c", Hash64: 32}
115 +
116 + rc.Store(a, 1, 1, []string{"chart-a"})
117 + rc.Store(b, 1, 1, []string{"chart-b"})
118 + rc.MarkSeenIfPresent(a, 2)
119 + rc.Store(c, 1, 1, []string{"chart-c"})
120 +
121 + stats := rc.RetainSeen(1)
122 + assert.Equal(t, 2, stats.EntriesBefore)
123 + assert.Equal(t, 2, stats.EntriesAfter)
124 + assert.Equal(t, 0, stats.Pruned)
125 + assert.False(t, stats.FullDrop)
126 +
127 + _, ok := rc.Lookup(c, 1, 2)
128 + assert.False(t, ok)
129 +
130 + stats = rc.RetainSeen(2)
131 + assert.Equal(t, 2, stats.EntriesBefore)
132 + assert.Equal(t, 1, stats.EntriesAfter)
133 + assert.Equal(t, 1, stats.Pruned)
134 + assert.False(t, stats.FullDrop)
135 +
136 + _, ok = rc.Lookup(a, 1, 2)
137 + assert.True(t, ok)
138 + _, ok = rc.Lookup(b, 1, 2)
139 + assert.False(t, ok)
140 + },
141 + },
142 + "retainSeenEntries clears truncated tail references": {
143 + run: func(t *testing.T) {
144 + bucket := []routeCacheEntry[string]{
145 + {
146 + identity: metrix.SeriesIdentity{ID: "a", Hash64: 40},
147 + revision: 1,
148 + values: []string{"chart-a"},
149 + lastSeenBuild: 2,
150 + },
151 + {
152 + identity: metrix.SeriesIdentity{ID: "b", Hash64: 41},
153 + revision: 1,
154 + values: []string{"chart-b"},
155 + lastSeenBuild: 1,
156 + },
157 + }
158 +
159 + kept := retainSeenEntries(bucket, 2)
160 + assert.Len(t, kept, 1)
161 + assert.Equal(t, metrix.SeriesID("a"), kept[0].identity.ID)
162 + assert.Equal(t, metrix.SeriesIdentity{}, bucket[1].identity)
163 + assert.Zero(t, bucket[1].revision)
164 + assert.Nil(t, bucket[1].values)
165 + assert.Zero(t, bucket[1].lastSeenBuild)
166 + },
167 + },
168 }
169
170 for name, tc := range tests {
src/go/plugin/framework/chartengine/internal/program/chart.go
+24 -9
@@ -2,7 +2,10 @@
2
3 package program
4
5 -import "fmt"
5 +import (
6 + "errors"
7 + "fmt"
8 +)
9
10 // Algorithm defines how Netdata interprets values on wire.
11 type Algorithm string
@@ -80,30 +83,42 @@ type ChartIdentity struct {
83 }
84
85 func validateChart(chart Chart) error {
86 + var errs []error
87 if chart.TemplateID == "" {
84 - return fmt.Errorf("template_id is required")
88 + errs = append(errs, fmt.Errorf("template_id is required"))
89 }
90 if chart.Meta.Context == "" {
87 - return fmt.Errorf("context is required")
91 + errs = append(errs, fmt.Errorf("context is required"))
92 }
93 if chart.Meta.Units == "" {
90 - return fmt.Errorf("units is required")
94 + errs = append(errs, fmt.Errorf("units is required"))
95 }
96 if chart.Meta.Algorithm != AlgorithmAbsolute && chart.Meta.Algorithm != AlgorithmIncremental {
93 - return fmt.Errorf("invalid algorithm %q", chart.Meta.Algorithm)
97 + errs = append(errs, fmt.Errorf("invalid algorithm %q", chart.Meta.Algorithm))
98 + }
99 + switch chart.Meta.Type {
100 + case ChartTypeLine, ChartTypeArea, ChartTypeStacked, ChartTypeHeatmap:
101 + default:
102 + errs = append(errs, fmt.Errorf("invalid chart type %q", chart.Meta.Type))
103 + }
104 + if err := validateInstanceLabelSelectors(chart.Identity.InstanceByLabels); err != nil {
105 + errs = append(errs, fmt.Errorf("identity: %w", err))
106 + }
107 + if err := validateLabelPolicy(chart.Labels); err != nil {
108 + errs = append(errs, fmt.Errorf("labels: %w", err))
109 }
110 if chart.CollisionReduce == "" {
96 - return fmt.Errorf("collision reduce op is required")
111 + errs = append(errs, fmt.Errorf("collision reduce op is required"))
112 }
113 if len(chart.Dimensions) == 0 {
99 - return fmt.Errorf("at least one dimension is required")
114 + errs = append(errs, fmt.Errorf("at least one dimension is required"))
115 }
116 for i, dim := range chart.Dimensions {
117 if err := validateDimension(dim); err != nil {
103 - return fmt.Errorf("dimension[%d]: %w", i, err)
118 + errs = append(errs, fmt.Errorf("dimension[%d]: %w", i, err))
119 }
120 }
106 - return nil
121 + return errors.Join(errs...)
122 }
123
124 func (c Chart) clone() Chart {
src/go/plugin/framework/chartengine/internal/program/dimension.go
+9 -5
@@ -2,7 +2,10 @@
2
3 package program
4
5 -import "fmt"
5 +import (
6 + "errors"
7 + "fmt"
8 +)
9
10 // SelectorMatcher is the runtime predicate compiled from selector expressions.
11 //
@@ -56,16 +59,17 @@ type Dimension struct {
59 }
60
61 func validateDimension(dimension Dimension) error {
62 + var errs []error
63 if dimension.Selector.Expression == "" {
60 - return fmt.Errorf("selector expression is required")
64 + errs = append(errs, fmt.Errorf("selector expression is required"))
65 }
66 if dimension.Selector.Matcher == nil {
63 - return fmt.Errorf("selector matcher is required")
67 + errs = append(errs, fmt.Errorf("selector matcher is required"))
68 }
69 if dimension.NameTemplate.Raw == "" && dimension.NameFromLabel == "" && !dimension.InferNameFromSeriesMeta {
66 - return fmt.Errorf("dimension name is required (name template or name_from_label)")
70 + errs = append(errs, fmt.Errorf("dimension name is required (name template or name_from_label)"))
71 }
68 - return nil
72 + return errors.Join(errs...)
73 }
74
75 func (d Dimension) clone() Dimension {
src/go/plugin/framework/chartengine/internal/program/labels.go
+54
@@ -2,6 +2,12 @@
2
3 package program
4
5 +import (
6 + "errors"
7 + "fmt"
8 + "strings"
9 +)
10 +
11 // PromotionMode defines how non-identity chart labels are selected.
12 type PromotionMode string
13
@@ -47,6 +53,54 @@ func DefaultLabelPrecedence() LabelPrecedence {
53 }
54 }
55
56 +func validateLabelPolicy(policy LabelPolicy) error {
57 + switch policy.Mode {
58 + case PromotionModeAutoIntersection, PromotionModeExplicitIntersection:
59 + return nil
60 + default:
61 + return fmt.Errorf("invalid promotion mode %q", policy.Mode)
62 + }
63 +}
64 +
65 +func validateInstanceLabelSelectors(selectors []InstanceLabelSelector) error {
66 + if len(selectors) == 0 {
67 + return nil
68 + }
69 +
70 + hasPositive := false
71 + var errs []error
72 + for i, selector := range selectors {
73 + switch {
74 + case selector.IncludeAll:
75 + if selector.Exclude || selector.Key != "" {
76 + errs = append(errs, fmt.Errorf("instance selector[%d]: include-all selector must not set exclude or key", i))
77 + continue
78 + }
79 + hasPositive = true
80 + case selector.Exclude:
81 + if selector.Key == "" {
82 + errs = append(errs, fmt.Errorf("instance selector[%d]: exclude selector key is required", i))
83 + continue
84 + }
85 + if strings.TrimSpace(selector.Key) != selector.Key {
86 + errs = append(errs, fmt.Errorf("instance selector[%d]: exclude selector key must be trimmed", i))
87 + }
88 + case selector.Key != "":
89 + if strings.TrimSpace(selector.Key) != selector.Key {
90 + errs = append(errs, fmt.Errorf("instance selector[%d]: selector key must be trimmed", i))
91 + }
92 + hasPositive = true
93 + default:
94 + errs = append(errs, fmt.Errorf("instance selector[%d]: selector is empty", i))
95 + }
96 + }
97 +
98 + if !hasPositive {
99 + errs = append(errs, fmt.Errorf("instance selectors must include at least one positive selector"))
100 + }
101 + return errors.Join(errs...)
102 +}
103 +
104 // InstanceLabelSelector is a normalized token from instances.by_labels.
105 type InstanceLabelSelector struct {
106 // IncludeAll corresponds to token "*".
src/go/plugin/framework/chartengine/internal/program/lifecycle.go
+1 -3
@@ -19,7 +19,5 @@ type DimensionLifecyclePolicy struct {
19 }
20
21 func (p LifecyclePolicy) clone() LifecyclePolicy {
22 - out := p
23 - out.Dimensions = p.Dimensions
24 - return out
22 + return p
23 }
src/go/plugin/framework/chartengine/internal/program/program_test.go
+97 -2
@@ -2,9 +2,9 @@
2
3 package program
4
5 -import "testing"
6 -
5 import (
6 + "testing"
7 +
8 "github.com/stretchr/testify/assert"
9 "github.com/stretchr/testify/require"
10 )
@@ -97,6 +97,59 @@ func TestNewProgramScenarios(t *testing.T) {
97 },
98 wantErr: true,
99 },
100 + "rejects invalid chart type": {
101 + version: "v1",
102 + metrics: []string{"windows_rx_total"},
103 + charts: []Chart{
104 + func() Chart {
105 + chart := sampleChart("invalid-type")
106 + chart.Meta.Type = ChartType("bars")
107 + return chart
108 + }(),
109 + },
110 + wantErr: true,
111 + },
112 + "rejects missing label promotion mode": {
113 + version: "v1",
114 + metrics: []string{"windows_rx_total"},
115 + charts: []Chart{
116 + func() Chart {
117 + chart := sampleChart("missing-label-mode")
118 + chart.Labels.Mode = ""
119 + return chart
120 + }(),
121 + },
122 + wantErr: true,
123 + },
124 + "rejects negation-only instance selectors": {
125 + version: "v1",
126 + metrics: []string{"windows_rx_total"},
127 + charts: []Chart{
128 + func() Chart {
129 + chart := sampleChart("negation-only-instance-selectors")
130 + chart.Identity.InstanceByLabels = []InstanceLabelSelector{
131 + {Exclude: true, Key: "nic"},
132 + }
133 + return chart
134 + }(),
135 + },
136 + wantErr: true,
137 + },
138 + "rejects malformed instance selector keys": {
139 + version: "v1",
140 + metrics: []string{"windows_rx_total"},
141 + charts: []Chart{
142 + func() Chart {
143 + chart := sampleChart("malformed-instance-selector")
144 + chart.Identity.InstanceByLabels = []InstanceLabelSelector{
145 + {Key: "nic"},
146 + {Exclude: true, Key: " host"},
147 + }
148 + return chart
149 + }(),
150 + },
151 + wantErr: true,
152 + },
153 }
154
155 for name, tc := range tests {
@@ -114,6 +167,48 @@ func TestNewProgramScenarios(t *testing.T) {
167 }
168 }
169
170 +func TestValidateInstanceLabelSelectorsReportsJoinedErrors(t *testing.T) {
171 + err := validateInstanceLabelSelectors([]InstanceLabelSelector{
172 + {Exclude: true, Key: " host"},
173 + {},
174 + })
175 +
176 + require.Error(t, err)
177 + assert.ErrorContains(t, err, "instance selector[0]: exclude selector key must be trimmed")
178 + assert.ErrorContains(t, err, "instance selector[1]: selector is empty")
179 + assert.ErrorContains(t, err, "instance selectors must include at least one positive selector")
180 +}
181 +
182 +func TestValidateDimensionReportsJoinedErrors(t *testing.T) {
183 + err := validateDimension(Dimension{})
184 +
185 + require.Error(t, err)
186 + assert.ErrorContains(t, err, "selector expression is required")
187 + assert.ErrorContains(t, err, "selector matcher is required")
188 + assert.ErrorContains(t, err, "dimension name is required")
189 +}
190 +
191 +func TestNewProgramReportsJoinedChartErrors(t *testing.T) {
192 + chart := sampleChart("invalid-multi")
193 + chart.Meta.Context = ""
194 + chart.Meta.Units = ""
195 + chart.Identity.InstanceByLabels = []InstanceLabelSelector{
196 + {Exclude: true, Key: "nic"},
197 + }
198 + chart.CollisionReduce = ""
199 + chart.Dimensions = []Dimension{{}}
200 +
201 + _, err := New("v1", 42, []string{"windows_rx_total"}, []Chart{chart})
202 + require.Error(t, err)
203 + assert.ErrorContains(t, err, "context is required")
204 + assert.ErrorContains(t, err, "units is required")
205 + assert.ErrorContains(t, err, "identity: instance selectors must include at least one positive selector")
206 + assert.ErrorContains(t, err, "collision reduce op is required")
207 + assert.ErrorContains(t, err, "dimension[0]: selector expression is required")
208 + assert.ErrorContains(t, err, "selector matcher is required")
209 + assert.ErrorContains(t, err, "dimension name is required")
210 +}
211 +
212 func sampleChart(templateID string) Chart {
213 return Chart{
214 TemplateID: templateID,
src/go/plugin/framework/chartengine/lifecycle.go
-2
@@ -25,7 +25,6 @@ type materializedChartState struct {
25
26 // materializedDimensionState tracks one materialized dimension in a chart.
27 type materializedDimensionState struct {
28 - name string
28 hidden bool
29 float bool
30 static bool
@@ -142,7 +141,6 @@ func (c *materializedChartState) ensureDimension(name string, state dimensionSta
141 return dim, false
142 }
143 dim = &materializedDimensionState{
145 - name: name,
144 hidden: state.hidden,
145 float: state.float,
146 static: state.static,
src/go/plugin/framework/chartengine/lifecycle_defaults.go
+1 -3
@@ -20,7 +20,5 @@ var defaultChartLifecyclePolicy = program.LifecyclePolicy{
20 }
21
22 func defaultChartLifecyclePolicyCopy() program.LifecyclePolicy {
23 - out := defaultChartLifecyclePolicy
24 - out.Dimensions = defaultChartLifecyclePolicy.Dimensions
25 - return out
23 + return defaultChartLifecyclePolicy
24 }
src/go/plugin/framework/chartengine/planner_labels.go
+21 -13
@@ -10,6 +10,8 @@ import (
10 "github.com/netdata/netdata/go/plugins/plugin/framework/chartengine/internal/program"
11 )
12
13 +const collectJobLabel = "_collect_job"
14 +
15 type compiledInstanceLabelPlan struct {
16 explicitKeys []string
17 explicitSet map[string]struct{}
@@ -90,7 +92,6 @@ func compileInstanceLabelPlan(identity program.ChartIdentity) compiledInstanceLa
92 excludeSet: make(map[string]struct{}),
93 }
94
93 - seenExplicit := make(map[string]struct{}, len(identity.InstanceByLabels))
95 for _, token := range identity.InstanceByLabels {
96 switch {
97 case token.Exclude:
@@ -99,19 +100,26 @@ func compileInstanceLabelPlan(identity program.ChartIdentity) compiledInstanceLa
100 }
101 case token.IncludeAll:
102 plan.includeAll = true
102 - case token.Key != "":
103 - key := token.Key
104 - if _, excluded := plan.excludeSet[key]; excluded {
105 - continue
106 - }
107 - if _, exists := seenExplicit[key]; exists {
108 - continue
109 - }
110 - seenExplicit[key] = struct{}{}
111 - plan.explicitKeys = append(plan.explicitKeys, key)
112 - plan.explicitSet[key] = struct{}{}
103 }
104 }
105 +
106 + seenExplicit := make(map[string]struct{}, len(identity.InstanceByLabels))
107 + for _, token := range identity.InstanceByLabels {
108 + if token.Exclude || token.IncludeAll || token.Key == "" {
109 + continue
110 + }
111 +
112 + key := token.Key
113 + if _, excluded := plan.excludeSet[key]; excluded {
114 + continue
115 + }
116 + if _, exists := seenExplicit[key]; exists {
117 + continue
118 + }
119 + seenExplicit[key] = struct{}{}
120 + plan.explicitKeys = append(plan.explicitKeys, key)
121 + plan.explicitSet[key] = struct{}{}
122 + }
123 return plan
124 }
125
@@ -284,6 +292,6 @@ func (a *chartLabelAccumulator) materialize() (map[string]string, error) {
292 }
293 out[key] = value
294 }
287 - delete(out, "_collect_job")
295 + delete(out, collectJobLabel)
296 return out, nil
297 }
src/go/plugin/framework/chartengine/planner_labels_test.go
+37 -4
@@ -18,7 +18,7 @@ func TestChartLabelAccumulatorIntersectsLabels(t *testing.T) {
18 "auto intersection keeps only common labels and excludes dimension key": {
19 observed: []map[string]string{
20 {
21 - "_collect_job": "mysql-local",
21 + collectJobLabel: "mysql-local",
22 "env": "prod",
23 "instance": "db1",
24 "mode": "read",
@@ -26,7 +26,7 @@ func TestChartLabelAccumulatorIntersectsLabels(t *testing.T) {
26 "selector_fixed": "x",
27 },
28 {
29 - "_collect_job": "mysql-local",
29 + collectJobLabel: "mysql-local",
30 "env": "prod",
31 "instance": "db1",
32 "mode": "write",
@@ -34,7 +34,7 @@ func TestChartLabelAccumulatorIntersectsLabels(t *testing.T) {
34 "selector_fixed": "x",
35 },
36 {
37 - "_collect_job": "mysql-local",
37 + collectJobLabel: "mysql-local",
38 "env": "prod",
39 "instance": "db1",
40 "mode": "read",
@@ -78,7 +78,40 @@ func TestChartLabelAccumulatorIntersectsLabels(t *testing.T) {
78 assert.Equal(t, tc.want, got)
79 assert.NotContains(t, got, "mode")
80 assert.NotContains(t, got, "selector_fixed")
81 - assert.NotContains(t, got, "_collect_job")
81 + assert.NotContains(t, got, collectJobLabel)
82 + })
83 + }
84 +}
85 +
86 +func TestCompileInstanceLabelPlanExcludeWinsRegardlessOfTokenOrder(t *testing.T) {
87 + tests := map[string]struct {
88 + selectors []program.InstanceLabelSelector
89 + }{
90 + "exclude after explicit": {
91 + selectors: []program.InstanceLabelSelector{
92 + {Key: "host"},
93 + {Exclude: true, Key: "host"},
94 + {IncludeAll: true},
95 + },
96 + },
97 + "exclude before explicit": {
98 + selectors: []program.InstanceLabelSelector{
99 + {Exclude: true, Key: "host"},
100 + {Key: "host"},
101 + {IncludeAll: true},
102 + },
103 + },
104 + }
105 +
106 + for name, tc := range tests {
107 + t.Run(name, func(t *testing.T) {
108 + plan := compileInstanceLabelPlan(program.ChartIdentity{
109 + InstanceByLabels: tc.selectors,
110 + })
111 + assert.True(t, plan.includeAll)
112 + assert.Empty(t, plan.explicitKeys)
113 + assert.Empty(t, plan.explicitSet)
114 + assert.Contains(t, plan.excludeSet, "host")
115 })
116 }
117 }
src/go/plugin/framework/chartengine/planner_lifecycle.go
-33
@@ -347,39 +347,6 @@ func collectExpiryRemovals(
347 return removeDims, removeCharts
348 }
349
350 -func orderedDimensionNamesFromState(dimensions map[string]dimensionState) []string {
351 - type staticEntry struct {
352 - name string
353 - order int
354 - }
355 - staticEntries := make([]staticEntry, 0, len(dimensions))
356 - dynamicNames := make([]string, 0, len(dimensions))
357 - for name, state := range dimensions {
358 - if state.static {
359 - staticEntries = append(staticEntries, staticEntry{
360 - name: name,
361 - order: state.order,
362 - })
363 - continue
364 - }
365 - dynamicNames = append(dynamicNames, name)
366 - }
367 - sort.Slice(staticEntries, func(i, j int) bool {
368 - if staticEntries[i].order != staticEntries[j].order {
369 - return staticEntries[i].order < staticEntries[j].order
370 - }
371 - return staticEntries[i].name < staticEntries[j].name
372 - })
373 -
374 - sort.Strings(dynamicNames)
375 - out := make([]string, 0, len(staticEntries)+len(dynamicNames))
376 - for _, entry := range staticEntries {
377 - out = append(out, entry.name)
378 - }
379 - out = append(out, dynamicNames...)
380 - return out
381 -}
382 -
350 func orderedObservedDimensionNames(entries map[string]*dimBuildEntry, seenSeq uint64) []string {
351 type staticEntry struct {
352 name string
src/go/plugin/framework/charttpl/README.md
+7 -2
@@ -504,7 +504,7 @@ charts:
504 | `algorithm` | string | no | inferred from metrics | `absolute` or `incremental`. If omitted, inferred from metric suffixes. |
505 | `type` | string | no | `line` | `line`, `area`, `stacked`, or `heatmap`. |
506 | `priority` | int | no | `70000` | Chart ordering priority in the dashboard (`0` = use engine default `70000`). |
507 -| `label_promotion` | array[string] | no | from `chart_defaults` | Labels to promote as chart labels (for filtering/grouping in UI). |
507 +| `label_promotion` | array[string] | no | from `chart_defaults` | Labels to promote as chart labels (for filtering/grouping in UI). Entries must be non-empty label keys. |
508 | `instances` | object | no | from `chart_defaults` | Instance identity policy (see [instances](#instances)). |
509 | `lifecycle` | object | no | | Instance/dimension cap and expiry (see [lifecycle](#lifecycle)). |
510 | `dimensions` | array | **yes** | | At least one dimension required (see [dimensions](#6-dimensions)). |
@@ -579,6 +579,9 @@ instances:
579 | `*` | Include all labels. |
580 | `!label_key` | Exclude this label (use with `*` to include all _except_...). |
581
582 +Excludes are order-independent and always win. For example, both `["host", "!host"]` and `["!host", "host"]` exclude `host`.
583 +When `instances` is set, `by_labels` must include at least one positive selector: `*` or `label_key`. Exclude tokens use strict `!label_key` syntax; `! host` is invalid.
584 +
585 **Example: One chart per host**
586
587 ```yaml
@@ -1015,8 +1018,10 @@ All rules below produce semantic validation errors unless noted:
1018 | `name` and `name_from_label` must not be whitespace-only | semantic |
1019 | Duplicate dimension `name` values within the same chart are rejected | semantic |
1020 | `instances.by_labels` must contain at least one token when `instances` is set | semantic |
1018 -| `instances.by_labels` exclude token must include label key (e.g., `!key`, not bare `!`) | semantic |
1021 +| `instances.by_labels` exclude token must use `!label_key` syntax | semantic |
1022 +| `instances.by_labels` must include at least one positive selector (`*` or `label_key`) | semantic |
1023 | `instances.by_labels` tokens must not be duplicated | semantic |
1024 +| `label_promotion[]` entries must not be empty or whitespace-only | semantic |
1025 | Lifecycle numeric fields must be `>= 0` | semantic |
1026 | `engine.autogen.max_type_id_len` must be `0` or `>= 4` | semantic |
1027 | Unknown YAML fields | decode error (strict unmarshal) |
src/go/plugin/framework/charttpl/config_schema.json
+4 -2
@@ -170,7 +170,8 @@
170 "label_promotion": {
171 "type": "array",
172 "items": {
173 - "type": "string"
173 + "type": "string",
174 + "pattern": "\\S"
175 }
176 },
177 "instances": {
@@ -195,7 +196,8 @@
196 "label_promotion": {
197 "type": "array",
198 "items": {
198 - "type": "string"
199 + "type": "string",
200 + "pattern": "\\S"
201 }
202 },
203 "instances": {
src/go/plugin/framework/charttpl/spec_test.go
+24 -1
@@ -163,8 +163,31 @@ func TestConfigSchemaJSON(t *testing.T) {
163 schema := ConfigSchemaJSON
164 require.NotEmpty(t, schema)
165
166 - var doc any
166 + var doc map[string]any
167 require.NoError(t, json.Unmarshal([]byte(schema), &doc))
168 +
169 + defs, ok := doc["$defs"].(map[string]any)
170 + require.True(t, ok)
171 +
172 + chart, ok := defs["chart"].(map[string]any)
173 + require.True(t, ok)
174 + chartProps, ok := chart["properties"].(map[string]any)
175 + require.True(t, ok)
176 + chartLabelPromotion, ok := chartProps["label_promotion"].(map[string]any)
177 + require.True(t, ok)
178 + chartLabelPromotionItems, ok := chartLabelPromotion["items"].(map[string]any)
179 + require.True(t, ok)
180 + assert.Equal(t, `\S`, chartLabelPromotionItems["pattern"])
181 +
182 + chartDefaults, ok := defs["chart_defaults"].(map[string]any)
183 + require.True(t, ok)
184 + defaultProps, ok := chartDefaults["properties"].(map[string]any)
185 + require.True(t, ok)
186 + defaultLabelPromotion, ok := defaultProps["label_promotion"].(map[string]any)
187 + require.True(t, ok)
188 + defaultLabelPromotionItems, ok := defaultLabelPromotion["items"].(map[string]any)
189 + require.True(t, ok)
190 + assert.Equal(t, `\S`, defaultLabelPromotionItems["pattern"])
191 }
192
193 func TestDecodeYAMLFileScenarios(t *testing.T) {
src/go/plugin/framework/charttpl/validate.go
+96 -69
@@ -3,6 +3,7 @@
3 package charttpl
4
5 import (
6 + "errors"
7 "fmt"
8 "slices"
9 "strings"
@@ -18,40 +19,37 @@ func (s *Spec) Validate() error {
19 if s == nil {
20 return semErr("", "nil spec")
21 }
22 + var errs []error
23 if s.Version != VersionV1 {
22 - return semErr("version", fmt.Sprintf("expected %q", VersionV1))
24 + errs = append(errs, semErr("version", fmt.Sprintf("expected %q", VersionV1)))
25 }
26 if len(s.Groups) == 0 {
25 - return semErr("groups", "groups[] is required")
26 - }
27 - if err := validateEngine(s.Engine); err != nil {
28 - return err
27 + errs = append(errs, semErr("groups", "groups[] is required"))
28 }
29 + errs = append(errs, validateEngine(s.Engine))
30
31 for i := range s.Groups {
32 - if err := validateGroup(s.Groups[i], fmt.Sprintf("groups[%d]", i), nil); err != nil {
33 - return err
34 - }
32 + errs = append(errs, validateGroup(s.Groups[i], fmt.Sprintf("groups[%d]", i), nil))
33 }
36 - return nil
34 + return errors.Join(errs...)
35 }
36
37 func validateGroup(group Group, path string, inheritedMetrics map[string]struct{}) error {
38 + var errs []error
39 if strings.TrimSpace(group.Family) == "" {
41 - return semErr(path+".family", "must not be empty")
42 - }
43 - if err := validateChartDefaults(group.ChartDefaults, path); err != nil {
44 - return err
40 + errs = append(errs, semErr(path+".family", "must not be empty"))
41 }
42 + errs = append(errs, validateChartDefaults(group.ChartDefaults, path))
43
44 ownMetrics := make(map[string]struct{}, len(group.Metrics))
45 for i, name := range group.Metrics {
46 name = strings.TrimSpace(name)
47 if name == "" {
51 - return semErr(fmt.Sprintf("%s.metrics[%d]", path, i), "metric name must not be empty")
48 + errs = append(errs, semErr(fmt.Sprintf("%s.metrics[%d]", path, i), "metric name must not be empty"))
49 + continue
50 }
51 if _, ok := ownMetrics[name]; ok {
54 - return semErr(fmt.Sprintf("%s.metrics[%d]", path, i), fmt.Sprintf("duplicate metric %q", name))
52 + errs = append(errs, semErr(fmt.Sprintf("%s.metrics[%d]", path, i), fmt.Sprintf("duplicate metric %q", name)))
53 }
54 ownMetrics[name] = struct{}{}
55 }
@@ -65,82 +63,82 @@ func validateGroup(group Group, path string, inheritedMetrics map[string]struct{
63 }
64
65 for i := range group.Charts {
68 - if err := validateChart(group.Charts[i], fmt.Sprintf("%s.charts[%d]", path, i), effective); err != nil {
69 - return err
70 - }
66 + errs = append(errs, validateChart(group.Charts[i], fmt.Sprintf("%s.charts[%d]", path, i), effective))
67 }
68 for i := range group.Groups {
73 - if err := validateGroup(group.Groups[i], fmt.Sprintf("%s.groups[%d]", path, i), effective); err != nil {
74 - return err
75 - }
69 + errs = append(errs, validateGroup(group.Groups[i], fmt.Sprintf("%s.groups[%d]", path, i), effective))
70 }
77 - return nil
71 + return errors.Join(errs...)
72 }
73
74 func validateChartDefaults(defaults *ChartDefaults, path string) error {
75 if defaults == nil {
76 return nil
77 }
84 - return validateInstances(defaults.Instances, path+".chart_defaults")
78 + return errors.Join(
79 + validateLabelPromotion(defaults.LabelPromoted, path+".chart_defaults.label_promotion"),
80 + validateInstances(defaults.Instances, path+".chart_defaults"),
81 + )
82 }
83
84 func validateChart(chart Chart, path string, effectiveMetrics map[string]struct{}) error {
88 - if err := validateChartCore(chart, path); err != nil {
89 - return err
90 - }
91 - if err := validateLifecycle(chart.Lifecycle, path); err != nil {
92 - return err
93 - }
94 - if err := validateInstances(chart.Instances, path); err != nil {
95 - return err
96 - }
97 - return validateDimensions(chart.Dimensions, path, effectiveMetrics)
85 + return errors.Join(
86 + validateChartCore(chart, path),
87 + validateLabelPromotion(chart.LabelPromoted, path+".label_promotion"),
88 + validateLifecycle(chart.Lifecycle, path),
89 + validateInstances(chart.Instances, path),
90 + validateDimensions(chart.Dimensions, path, effectiveMetrics),
91 + )
92 }
93
94 func validateChartCore(chart Chart, path string) error {
95 + var errs []error
96 if strings.TrimSpace(chart.Title) == "" {
102 - return semErr(path+".title", "must not be empty")
97 + errs = append(errs, semErr(path+".title", "must not be empty"))
98 }
99 if strings.TrimSpace(chart.Context) == "" {
105 - return semErr(path+".context", "must not be empty")
100 + errs = append(errs, semErr(path+".context", "must not be empty"))
101 }
102 if strings.TrimSpace(chart.Units) == "" {
108 - return semErr(path+".units", "must not be empty")
103 + errs = append(errs, semErr(path+".units", "must not be empty"))
104 }
105 if chart.Algorithm != "" && !slices.Contains(validAlgorithms, chart.Algorithm) {
111 - return semErr(path+".algorithm", fmt.Sprintf("must be one of %v", validAlgorithms))
106 + errs = append(errs, semErr(path+".algorithm", fmt.Sprintf("must be one of %v", validAlgorithms)))
107 }
108 if chart.Type != "" && !slices.Contains(validChartTypes, chart.Type) {
114 - return semErr(path+".type", fmt.Sprintf("must be one of %v", validChartTypes))
109 + errs = append(errs, semErr(path+".type", fmt.Sprintf("must be one of %v", validChartTypes)))
110 }
116 - return nil
111 + return errors.Join(errs...)
112 }
113
114 func validateLifecycle(lifecycle *Lifecycle, path string) error {
115 if lifecycle == nil {
116 return nil
117 }
118 + var errs []error
119 if lifecycle.MaxInstances < 0 {
124 - return semErr(path+".lifecycle.max_instances", "must be >= 0")
120 + errs = append(errs, semErr(path+".lifecycle.max_instances", "must be >= 0"))
121 }
122 if lifecycle.ExpireAfterCycles < 0 {
127 - return semErr(path+".lifecycle.expire_after_cycles", "must be >= 0")
123 + errs = append(errs, semErr(path+".lifecycle.expire_after_cycles", "must be >= 0"))
124 }
125 if lifecycle.Dimensions != nil {
126 if lifecycle.Dimensions.MaxDims < 0 {
131 - return semErr(path+".lifecycle.dimensions.max_dims", "must be >= 0")
127 + errs = append(errs, semErr(path+".lifecycle.dimensions.max_dims", "must be >= 0"))
128 }
129 if lifecycle.Dimensions.ExpireAfterCycles < 0 {
134 - return semErr(path+".lifecycle.dimensions.expire_after_cycles", "must be >= 0")
130 + errs = append(errs, semErr(path+".lifecycle.dimensions.expire_after_cycles", "must be >= 0"))
131 }
132 }
137 - return nil
133 + return errors.Join(errs...)
134 }
135
136 func validateInstances(instances *Instances, path string) error {
137 if instances == nil {
138 return nil
139 }
140 + var errs []error
141 + hasPositive := false
142 if len(instances.ByLabels) == 0 {
143 return semErr(path+".instances.by_labels", "must contain at least one token when instances is set")
144 }
@@ -149,17 +147,34 @@ func validateInstances(instances *Instances, path string) error {
147 for i, token := range instances.ByLabels {
148 token = strings.TrimSpace(token)
149 if token == "" {
152 - return semErr(fmt.Sprintf("%s.instances.by_labels[%d]", path, i), "must not be empty")
150 + errs = append(errs, semErr(fmt.Sprintf("%s.instances.by_labels[%d]", path, i), "must not be empty"))
151 + continue
152 }
154 - if token != "*" && strings.HasPrefix(token, "!") && len(token) == 1 {
155 - return semErr(fmt.Sprintf("%s.instances.by_labels[%d]", path, i), "exclude token must include label key")
153 + switch {
154 + case token == "*":
155 + hasPositive = true
156 + case strings.HasPrefix(token, "!"):
157 + key := strings.TrimPrefix(token, "!")
158 + if key == "" {
159 + errs = append(errs, semErr(fmt.Sprintf("%s.instances.by_labels[%d]", path, i), "exclude token must include label key"))
160 + continue
161 + }
162 + if strings.TrimSpace(key) != key {
163 + errs = append(errs, semErr(fmt.Sprintf("%s.instances.by_labels[%d]", path, i), "exclude token must use !label_key syntax"))
164 + continue
165 + }
166 + default:
167 + hasPositive = true
168 }
169 if _, ok := seen[token]; ok {
158 - return semErr(fmt.Sprintf("%s.instances.by_labels[%d]", path, i), fmt.Sprintf("duplicate token %q", token))
170 + errs = append(errs, semErr(fmt.Sprintf("%s.instances.by_labels[%d]", path, i), fmt.Sprintf("duplicate token %q", token)))
171 }
172 seen[token] = struct{}{}
173 }
162 - return nil
174 + if !hasPositive {
175 + errs = append(errs, semErr(path+".instances.by_labels", "must include at least one positive selector ('*' or label key)"))
176 + }
177 + return errors.Join(errs...)
178 }
179
180 func validateDimensions(dimensions []Dimension, path string, effectiveMetrics map[string]struct{}) error {
@@ -167,41 +182,42 @@ func validateDimensions(dimensions []Dimension, path string, effectiveMetrics ma
182 return semErr(path+".dimensions", "at least one dimension is required")
183 }
184
185 + var errs []error
186 seenDimNames := make(map[string]struct{}, len(dimensions))
187 for i := range dimensions {
188 d := dimensions[i]
189 selectorExpr := strings.TrimSpace(d.Selector)
190 if selectorExpr == "" {
175 - return semErr(fmt.Sprintf("%s.dimensions[%d].selector", path, i), "must not be empty")
176 - }
177 -
178 - metricName, ok := selectorMetricName(selectorExpr)
179 - if !ok {
180 - return semErr(fmt.Sprintf("%s.dimensions[%d].selector", path, i), "selector must include explicit metric name")
181 - }
182 - if _, ok := effectiveMetrics[metricName]; !ok {
183 - return semErr(fmt.Sprintf("%s.dimensions[%d].selector", path, i), fmt.Sprintf("metric %q is not visible in current group scope", metricName))
191 + errs = append(errs, semErr(fmt.Sprintf("%s.dimensions[%d].selector", path, i), "must not be empty"))
192 + } else {
193 + metricName, ok := selectorMetricName(selectorExpr)
194 + if !ok {
195 + errs = append(errs, semErr(fmt.Sprintf("%s.dimensions[%d].selector", path, i), "selector must include explicit metric name"))
196 + } else if _, ok := effectiveMetrics[metricName]; !ok {
197 + errs = append(errs, semErr(fmt.Sprintf("%s.dimensions[%d].selector", path, i), fmt.Sprintf("metric %q is not visible in current group scope", metricName)))
198 + }
199 }
200
201 name := strings.TrimSpace(d.Name)
202 nameFrom := strings.TrimSpace(d.NameFromLabel)
203 if d.Name != "" && name == "" {
189 - return semErr(fmt.Sprintf("%s.dimensions[%d].name", path, i), "must not be whitespace-only")
204 + errs = append(errs, semErr(fmt.Sprintf("%s.dimensions[%d].name", path, i), "must not be whitespace-only"))
205 }
206 if d.NameFromLabel != "" && nameFrom == "" {
192 - return semErr(fmt.Sprintf("%s.dimensions[%d].name_from_label", path, i), "must not be whitespace-only")
207 + errs = append(errs, semErr(fmt.Sprintf("%s.dimensions[%d].name_from_label", path, i), "must not be whitespace-only"))
208 }
209 if name != "" && nameFrom != "" {
195 - return semErr(fmt.Sprintf("%s.dimensions[%d]", path, i), "use either name or name_from_label, not both")
210 + errs = append(errs, semErr(fmt.Sprintf("%s.dimensions[%d]", path, i), "use either name or name_from_label, not both"))
211 }
212 if name != "" {
213 if _, ok := seenDimNames[name]; ok {
199 - return semErr(fmt.Sprintf("%s.dimensions[%d].name", path, i), fmt.Sprintf("duplicate dimension name %q", name))
214 + errs = append(errs, semErr(fmt.Sprintf("%s.dimensions[%d].name", path, i), fmt.Sprintf("duplicate dimension name %q", name)))
215 + } else {
216 + seenDimNames[name] = struct{}{}
217 }
201 - seenDimNames[name] = struct{}{}
218 }
219 }
204 - return nil
220 + return errors.Join(errs...)
221 }
222
223 func validateEngine(engine *Engine) error {
@@ -209,29 +225,40 @@ func validateEngine(engine *Engine) error {
225 return nil
226 }
227
228 + var errs []error
229 if engine.Selector != nil {
230 for i, expr := range engine.Selector.Allow {
231 if strings.TrimSpace(expr) == "" {
215 - return semErr(fmt.Sprintf("engine.selector.allow[%d]", i), "must not be empty")
232 + errs = append(errs, semErr(fmt.Sprintf("engine.selector.allow[%d]", i), "must not be empty"))
233 }
234 }
235 for i, expr := range engine.Selector.Deny {
236 if strings.TrimSpace(expr) == "" {
220 - return semErr(fmt.Sprintf("engine.selector.deny[%d]", i), "must not be empty")
237 + errs = append(errs, semErr(fmt.Sprintf("engine.selector.deny[%d]", i), "must not be empty"))
238 }
239 }
240 }
241
242 if engine.Autogen != nil {
243 if engine.Autogen.MaxTypeIDLen < 0 {
227 - return semErr("engine.autogen.max_type_id_len", "must be >= 0")
244 + errs = append(errs, semErr("engine.autogen.max_type_id_len", "must be >= 0"))
245 }
246 if engine.Autogen.MaxTypeIDLen > 0 && engine.Autogen.MaxTypeIDLen < 4 {
230 - return semErr("engine.autogen.max_type_id_len", "must be >= 4 when set")
247 + errs = append(errs, semErr("engine.autogen.max_type_id_len", "must be >= 4 when set"))
248 }
249 }
250
234 - return nil
251 + return errors.Join(errs...)
252 +}
253 +
254 +func validateLabelPromotion(labels []string, path string) error {
255 + var errs []error
256 + for i, label := range labels {
257 + if strings.TrimSpace(label) == "" {
258 + errs = append(errs, semErr(fmt.Sprintf("%s[%d]", path, i), "must not be empty"))
259 + }
260 + }
261 + return errors.Join(errs...)
262 }
263
264 func selectorMetricName(expr string) (string, bool) {
src/go/plugin/framework/charttpl/validate_test.go
+229
@@ -3,6 +3,7 @@
3 package charttpl
4
5 import (
6 + "errors"
7 "testing"
8
9 "github.com/stretchr/testify/assert"
@@ -829,3 +830,231 @@ func TestSpecValidateNilAndVersion(t *testing.T) {
830 require.Error(t, err)
831 assert.ErrorContains(t, err, "expected \"v1\"")
832 }
833 +
834 +func TestSpecValidateReportsAllSemanticErrors(t *testing.T) {
835 + spec := Spec{
836 + Version: VersionV1,
837 + Groups: []Group{
838 + {
839 + Family: "Database",
840 + ChartDefaults: &ChartDefaults{
841 + LabelPromoted: []string{"cluster", " "},
842 + },
843 + Metrics: []string{"mysql_queries_total"},
844 + Charts: []Chart{
845 + {
846 + Title: "Queries",
847 + Context: "queries_total",
848 + Units: "queries/s",
849 + LabelPromoted: []string{"instance", ""},
850 + Instances: &Instances{ByLabels: []string{"*", "*"}},
851 + Dimensions: []Dimension{{Selector: "mysql_queries_total", Name: "total"}},
852 + },
853 + },
854 + },
855 + },
856 + }
857 +
858 + err := spec.Validate()
859 + require.Error(t, err)
860 + assert.True(t, errors.Is(err, errSemanticCheck))
861 + assert.ErrorContains(t, err, "groups[0].chart_defaults.label_promotion[1]")
862 + assert.ErrorContains(t, err, "groups[0].charts[0].label_promotion[1]")
863 + assert.ErrorContains(t, err, "groups[0].charts[0].instances.by_labels[1]")
864 +}
865 +
866 +func TestSpecValidateRejectsEmptyLabelPromotionEntries(t *testing.T) {
867 + tests := map[string]struct {
868 + spec Spec
869 + errLike string
870 + }{
871 + "chart label_promotion": {
872 + spec: Spec{
873 + Version: VersionV1,
874 + Groups: []Group{
875 + {
876 + Family: "Database",
877 + Metrics: []string{"mysql_queries_total"},
878 + Charts: []Chart{
879 + {
880 + Title: "Queries",
881 + Context: "queries_total",
882 + Units: "queries/s",
883 + LabelPromoted: []string{"cluster", " "},
884 + Dimensions: []Dimension{{Selector: "mysql_queries_total", Name: "total"}},
885 + },
886 + },
887 + },
888 + },
889 + },
890 + errLike: "groups[0].charts[0].label_promotion[1]",
891 + },
892 + "chart_defaults label_promotion": {
893 + spec: Spec{
894 + Version: VersionV1,
895 + Groups: []Group{
896 + {
897 + Family: "Database",
898 + ChartDefaults: &ChartDefaults{
899 + LabelPromoted: []string{"cluster", ""},
900 + },
901 + Metrics: []string{"mysql_queries_total"},
902 + Charts: []Chart{
903 + {
904 + Title: "Queries",
905 + Context: "queries_total",
906 + Units: "queries/s",
907 + Dimensions: []Dimension{{Selector: "mysql_queries_total", Name: "total"}},
908 + },
909 + },
910 + },
911 + },
912 + },
913 + errLike: "groups[0].chart_defaults.label_promotion[1]",
914 + },
915 + }
916 +
917 + for name, tc := range tests {
918 + t.Run(name, func(t *testing.T) {
919 + err := tc.spec.Validate()
920 + require.Error(t, err)
921 + assert.ErrorContains(t, err, tc.errLike)
922 + })
923 + }
924 +}
925 +
926 +func TestSpecValidateRejectsMalformedExcludeTokens(t *testing.T) {
927 + tests := map[string]struct {
928 + spec Spec
929 + errLike string
930 + }{
931 + "chart instances": {
932 + spec: Spec{
933 + Version: VersionV1,
934 + Groups: []Group{
935 + {
936 + Family: "Database",
937 + Metrics: []string{"mysql_queries_total"},
938 + Charts: []Chart{
939 + {
940 + Title: "Queries",
941 + Context: "queries_total",
942 + Units: "queries/s",
943 + Instances: &Instances{
944 + ByLabels: []string{"*", "! host"},
945 + },
946 + Dimensions: []Dimension{
947 + {Selector: "mysql_queries_total", Name: "total"},
948 + },
949 + },
950 + },
951 + },
952 + },
953 + },
954 + errLike: "exclude token must use !label_key syntax",
955 + },
956 + "chart_defaults instances": {
957 + spec: Spec{
958 + Version: VersionV1,
959 + Groups: []Group{
960 + {
961 + Family: "Database",
962 + ChartDefaults: &ChartDefaults{
963 + Instances: &Instances{
964 + ByLabels: []string{"*", "! host"},
965 + },
966 + },
967 + Metrics: []string{"mysql_queries_total"},
968 + Charts: []Chart{
969 + {
970 + Title: "Queries",
971 + Context: "queries_total",
972 + Units: "queries/s",
973 + Dimensions: []Dimension{
974 + {Selector: "mysql_queries_total", Name: "total"},
975 + },
976 + },
977 + },
978 + },
979 + },
980 + },
981 + errLike: "chart_defaults.instances.by_labels[1]",
982 + },
983 + }
984 +
985 + for name, tc := range tests {
986 + t.Run(name, func(t *testing.T) {
987 + err := tc.spec.Validate()
988 + require.Error(t, err)
989 + assert.ErrorContains(t, err, tc.errLike)
990 + })
991 + }
992 +}
993 +
994 +func TestSpecValidateRejectsNegationOnlyInstances(t *testing.T) {
995 + tests := map[string]struct {
996 + spec Spec
997 + errLike string
998 + }{
999 + "chart instances": {
1000 + spec: Spec{
1001 + Version: VersionV1,
1002 + Groups: []Group{
1003 + {
1004 + Family: "Database",
1005 + Metrics: []string{"mysql_queries_total"},
1006 + Charts: []Chart{
1007 + {
1008 + Title: "Queries",
1009 + Context: "queries_total",
1010 + Units: "queries/s",
1011 + Instances: &Instances{
1012 + ByLabels: []string{"!host"},
1013 + },
1014 + Dimensions: []Dimension{
1015 + {Selector: "mysql_queries_total", Name: "total"},
1016 + },
1017 + },
1018 + },
1019 + },
1020 + },
1021 + },
1022 + errLike: "must include at least one positive selector",
1023 + },
1024 + "chart_defaults instances": {
1025 + spec: Spec{
1026 + Version: VersionV1,
1027 + Groups: []Group{
1028 + {
1029 + Family: "Database",
1030 + ChartDefaults: &ChartDefaults{
1031 + Instances: &Instances{
1032 + ByLabels: []string{"!host"},
1033 + },
1034 + },
1035 + Metrics: []string{"mysql_queries_total"},
1036 + Charts: []Chart{
1037 + {
1038 + Title: "Queries",
1039 + Context: "queries_total",
1040 + Units: "queries/s",
1041 + Dimensions: []Dimension{
1042 + {Selector: "mysql_queries_total", Name: "total"},
1043 + },
1044 + },
1045 + },
1046 + },
1047 + },
1048 + },
1049 + errLike: "chart_defaults.instances.by_labels",
1050 + },
1051 + }
1052 +
1053 + for name, tc := range tests {
1054 + t.Run(name, func(t *testing.T) {
1055 + err := tc.spec.Validate()
1056 + require.Error(t, err)
1057 + assert.ErrorContains(t, err, tc.errLike)
1058 + })
1059 + }
1060 +}