From e68e9ba9392ce8898f8430338a26ddec0ea6174d Mon Sep 17 00:00:00 2001 From: Smyslov Maxim Date: Fri, 14 Aug 2026 16:01:21 +0300 Subject: [PATCH 1/4] refactor: change Operation methods to use pointer receivers and update return types for MetricOperations functions Signed-off-by: Smyslov Maxim --- pkg/metric/operation/operation.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/pkg/metric/operation/operation.go b/pkg/metric/operation/operation.go index f259e6b4..e5b89387 100644 --- a/pkg/metric/operation/operation.go +++ b/pkg/metric/operation/operation.go @@ -36,11 +36,11 @@ type Operation struct { Labels map[string]string `json:"labels"` } -func (op Operation) WithGroup(group string) { - op.Group = group //nolint: staticcheck +func (op *Operation) WithGroup(group string) { + op.Group = group } -func (op Operation) Validate() error { +func (op *Operation) Validate() error { var err error if op.Action == "" { @@ -80,13 +80,13 @@ func (op Operation) Validate() error { return err } -func MetricOperationsFromReader(r io.Reader) ([]Operation, error) { - operations := make([]Operation, 0) +func MetricOperationsFromReader(r io.Reader) ([]*Operation, error) { + operations := make([]*Operation, 0) dec := json.NewDecoder(r) for { - var metricOperation Operation - if err := dec.Decode(&metricOperation); err == io.EOF { + var metricOperation *Operation + if err := dec.Decode(metricOperation); err == io.EOF { break } else if err != nil { return nil, err @@ -98,11 +98,11 @@ func MetricOperationsFromReader(r io.Reader) ([]Operation, error) { return operations, nil } -func MetricOperationsFromBytes(data []byte) ([]Operation, error) { +func MetricOperationsFromBytes(data []byte) ([]*Operation, error) { return MetricOperationsFromReader(bytes.NewReader(data)) } -func MetricOperationsFromFile(filePath string) ([]Operation, error) { +func MetricOperationsFromFile(filePath string) ([]*Operation, error) { data, err := os.ReadFile(filePath) if err != nil { return nil, fmt.Errorf("cannot read %s: %s", filePath, err) @@ -113,8 +113,7 @@ func MetricOperationsFromFile(filePath string) ([]Operation, error) { } return MetricOperationsFromBytes(data) } - -func ValidateOperations(ops []Operation) error { +func ValidateOperations(ops []*Operation) error { var opsErrs error for _, op := range ops { From 2a04ea54f7123ae1d8ecf3db92f9f805a40f40df Mon Sep 17 00:00:00 2001 From: Smyslov Maxim Date: Fri, 14 Aug 2026 16:19:32 +0300 Subject: [PATCH 2/4] fix Signed-off-by: Smyslov Maxim --- pkg/metric/operation/operation.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/metric/operation/operation.go b/pkg/metric/operation/operation.go index e5b89387..0436efc1 100644 --- a/pkg/metric/operation/operation.go +++ b/pkg/metric/operation/operation.go @@ -85,7 +85,7 @@ func MetricOperationsFromReader(r io.Reader) ([]*Operation, error) { dec := json.NewDecoder(r) for { - var metricOperation *Operation + metricOperation := new(Operation) if err := dec.Decode(metricOperation); err == io.EOF { break } else if err != nil { @@ -113,6 +113,7 @@ func MetricOperationsFromFile(filePath string) ([]*Operation, error) { } return MetricOperationsFromBytes(data) } + func ValidateOperations(ops []*Operation) error { var opsErrs error From 8919e53bd0f9ca9bafeaa85e9b257db089cb3082 Mon Sep 17 00:00:00 2001 From: Smyslov Maxim Date: Fri, 14 Aug 2026 16:19:36 +0300 Subject: [PATCH 3/4] add test Signed-off-by: Smyslov Maxim --- internal/metric/collector_test.go | 90 +++++++++++++++++++ pkg/metric/operation/operation_test.go | 2 +- .../operation/operation_withgroup_test.go | 60 +++++++++++++ 3 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 internal/metric/collector_test.go create mode 100644 pkg/metric/operation/operation_withgroup_test.go diff --git a/internal/metric/collector_test.go b/internal/metric/collector_test.go new file mode 100644 index 00000000..f8654001 --- /dev/null +++ b/internal/metric/collector_test.go @@ -0,0 +1,90 @@ +package metric + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + operation "github.com/deckhouse/module-sdk/pkg/metric/operation" +) + +// Set(..., WithGroup("g")) must land the group on the collected operation. +// This is the end-to-end path a Go hook actually uses. +func Test_Collector_Set_WithGroup(t *testing.T) { + mc := NewCollector() + + mc.Set("d8_example_metric", 1, nil, operation.WithGroup("example_group")) + + metrics := mc.CollectedMetrics() + require.Len(t, metrics, 1) + assert.Equal(t, "example_group", metrics[0].Group) + assert.Equal(t, "set", metrics[0].Action) +} + +func Test_Collector_Add_WithGroup(t *testing.T) { + mc := NewCollector() + + mc.Add("d8_example_metric", 2, nil, operation.WithGroup("example_group")) + + metrics := mc.CollectedMetrics() + require.Len(t, metrics, 1) + assert.Equal(t, "example_group", metrics[0].Group) + assert.Equal(t, "add", metrics[0].Action) +} + +func Test_Collector_Inc_WithGroup(t *testing.T) { + mc := NewCollector() + + mc.Inc("d8_example_metric", nil, operation.WithGroup("example_group")) + + metrics := mc.CollectedMetrics() + require.Len(t, metrics, 1) + assert.Equal(t, "example_group", metrics[0].Group) +} + +// WithGroup must override the collector's default group. +func Test_Collector_WithGroup_OverridesDefaultGroup(t *testing.T) { + mc := NewCollector(WithDefaultGroup("default_group")) + + mc.Set("d8_example_metric", 1, nil, operation.WithGroup("explicit_group")) + + metrics := mc.CollectedMetrics() + require.Len(t, metrics, 1) + assert.Equal(t, "explicit_group", metrics[0].Group) +} + +// Without an explicit group the default group is used. +func Test_Collector_DefaultGroup_Applied(t *testing.T) { + mc := NewCollector(WithDefaultGroup("default_group")) + + mc.Set("d8_example_metric", 1, nil) + + metrics := mc.CollectedMetrics() + require.Len(t, metrics, 1) + assert.Equal(t, "default_group", metrics[0].Group) +} + +// Applying WithGroup to one operation must not leak into a later one. +func Test_Collector_WithGroup_DoesNotLeakBetweenMetrics(t *testing.T) { + mc := NewCollector() + + mc.Set("grouped", 1, nil, operation.WithGroup("g1")) + mc.Set("ungrouped", 1, nil) + + metrics := mc.CollectedMetrics() + require.Len(t, metrics, 2) + assert.Equal(t, "g1", metrics[0].Group) + assert.Empty(t, metrics[1].Group) +} + +func Test_Collector_Expire(t *testing.T) { + mc := NewCollector() + + mc.Expire("some_group") + + metrics := mc.CollectedMetrics() + require.Len(t, metrics, 1) + assert.Equal(t, "some_group", metrics[0].Group) + assert.Equal(t, "expire", metrics[0].Action) +} diff --git a/pkg/metric/operation/operation_test.go b/pkg/metric/operation/operation_test.go index bc2da34d..8de0cd2d 100644 --- a/pkg/metric/operation/operation_test.go +++ b/pkg/metric/operation/operation_test.go @@ -48,7 +48,7 @@ func Test_ValidateOperations(t *testing.T) { Action: "expired", Group: "someGroup", }, - errors.New("'name' is required when action is not 'expire': {Name: Group:someGroup Action:expired Value: Buckets:[] Labels:map[]}"), + errors.New("'name' is required when action is not 'expire': &{Name: Group:someGroup Action:expired Value: Buckets:[] Labels:map[]}"), }, } diff --git a/pkg/metric/operation/operation_withgroup_test.go b/pkg/metric/operation/operation_withgroup_test.go new file mode 100644 index 00000000..459e2928 --- /dev/null +++ b/pkg/metric/operation/operation_withgroup_test.go @@ -0,0 +1,60 @@ +package operation + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/deckhouse/module-sdk/pkg" +) + +// Regression test: WithGroup used to be declared on a value receiver, so the +// assignment landed in a copy and the group was silently dropped. +func Test_Operation_WithGroup_MutatesReceiver(t *testing.T) { + op := &Operation{Name: "d8_example_metric", Action: "set"} + + op.WithGroup("example_group") + + assert.Equal(t, "example_group", op.Group) +} + +// The option produced by the package-level WithGroup must reach the operation +// through the applier interface, exactly as the collector invokes it. +func Test_WithGroup_Option_AppliesThroughApplier(t *testing.T) { + op := &Operation{Name: "d8_example_metric", Action: "set"} + + var applier pkg.MetricCollectorOptionApplier = op + WithGroup("example_group").Apply(applier) + + assert.Equal(t, "example_group", op.Group) +} + +// *Operation must keep satisfying the applier interface after the receiver +// change, otherwise the collector would fail to compile. +func Test_Operation_ImplementsApplier(t *testing.T) { + var _ pkg.MetricCollectorOptionApplier = (*Operation)(nil) +} + +func Test_MetricOperationsFromReader_RoundTrip(t *testing.T) { + data := []byte(`{"name":"m1","group":"g1","action":"set","value":1}` + "\n" + + `{"name":"m2","action":"add","value":2}` + "\n") + + ops, err := MetricOperationsFromBytes(data) + require.NoError(t, err) + require.Len(t, ops, 2) + + assert.Equal(t, "m1", ops[0].Name) + assert.Equal(t, "g1", ops[0].Group) + assert.Equal(t, "set", ops[0].Action) + + assert.Equal(t, "m2", ops[1].Name) + assert.Empty(t, ops[1].Group) + assert.Equal(t, "add", ops[1].Action) +} + +func Test_MetricOperationsFromBytes_Empty(t *testing.T) { + ops, err := MetricOperationsFromBytes(nil) + require.NoError(t, err) + assert.Empty(t, ops) +} From a1bfb364ee23a3dd6977440e24706782c191e639 Mon Sep 17 00:00:00 2001 From: Smyslov Maxim Date: Fri, 14 Aug 2026 17:33:38 +0300 Subject: [PATCH 4/4] fix Signed-off-by: Smyslov Maxim --- pkg/metric/operation/operation.go | 16 ++++++++-------- pkg/metric/operation/operation_test.go | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/metric/operation/operation.go b/pkg/metric/operation/operation.go index 0436efc1..df5f09be 100644 --- a/pkg/metric/operation/operation.go +++ b/pkg/metric/operation/operation.go @@ -40,7 +40,7 @@ func (op *Operation) WithGroup(group string) { op.Group = group } -func (op *Operation) Validate() error { +func (op Operation) Validate() error { var err error if op.Action == "" { @@ -80,13 +80,13 @@ func (op *Operation) Validate() error { return err } -func MetricOperationsFromReader(r io.Reader) ([]*Operation, error) { - operations := make([]*Operation, 0) +func MetricOperationsFromReader(r io.Reader) ([]Operation, error) { + operations := make([]Operation, 0) dec := json.NewDecoder(r) for { - metricOperation := new(Operation) - if err := dec.Decode(metricOperation); err == io.EOF { + var metricOperation Operation + if err := dec.Decode(&metricOperation); err == io.EOF { break } else if err != nil { return nil, err @@ -98,11 +98,11 @@ func MetricOperationsFromReader(r io.Reader) ([]*Operation, error) { return operations, nil } -func MetricOperationsFromBytes(data []byte) ([]*Operation, error) { +func MetricOperationsFromBytes(data []byte) ([]Operation, error) { return MetricOperationsFromReader(bytes.NewReader(data)) } -func MetricOperationsFromFile(filePath string) ([]*Operation, error) { +func MetricOperationsFromFile(filePath string) ([]Operation, error) { data, err := os.ReadFile(filePath) if err != nil { return nil, fmt.Errorf("cannot read %s: %s", filePath, err) @@ -114,7 +114,7 @@ func MetricOperationsFromFile(filePath string) ([]*Operation, error) { return MetricOperationsFromBytes(data) } -func ValidateOperations(ops []*Operation) error { +func ValidateOperations(ops []Operation) error { var opsErrs error for _, op := range ops { diff --git a/pkg/metric/operation/operation_test.go b/pkg/metric/operation/operation_test.go index 8de0cd2d..bc2da34d 100644 --- a/pkg/metric/operation/operation_test.go +++ b/pkg/metric/operation/operation_test.go @@ -48,7 +48,7 @@ func Test_ValidateOperations(t *testing.T) { Action: "expired", Group: "someGroup", }, - errors.New("'name' is required when action is not 'expire': &{Name: Group:someGroup Action:expired Value: Buckets:[] Labels:map[]}"), + errors.New("'name' is required when action is not 'expire': {Name: Group:someGroup Action:expired Value: Buckets:[] Labels:map[]}"), }, }