processor/deltatocumulative: telemetry tests #35742

sh0rez · 2024-10-11T12:49:36Z

Description

Tests internal telemetry (metadata.TelemetryBuilder) is recorded as
expected.

Introduces internal/testing/sdktest for this.
Introduces -- telemetry -- section to testdata.

Testing

Existing tests were extended to have a -- telemetry -- section that specifies expected meter readings in sdktest.Format

Documentation

not needed

sh0rez · 2024-10-18T15:50:28Z

processor/deltatocumulativeprocessor/internal/lineartelemetry/metrics.go

@@ -36,15 +37,15 @@ func New(set component.TelemetrySettings) (Metrics, error) {
 type Metrics struct {
 	metadata.TelemetryBuilder

-	tracked func() int
+	tracked *func() int


fixes a bug discovered during testing where the callback update had no effect, so the metric was always 0

Are you sure this is correct? tracked func() int is a function pointer already. tracked *func() int is a pointer to a pointer.

in-fact yes.

opentelemetry-collector-contrib/processor/deltatocumulativeprocessor/internal/lineartelemetry/metrics.go

Lines 20 to 26 in e57fcf4

m := Metrics{

tracked: &zero,

}

trackedCb := metadata.WithDeltatocumulativeStreamsTrackedLinearCallback(func() int64 {

return int64((*m.tracked)())

})

trackedCb captures m.tracked by value. if we later re-assign that func(), this is not reflected here.
by having it capture a pointer instead, we can update the underlying value and the closure correctly sees the new callback

Would it make sense to use separate commits here? One that adds the test, even if it fails, another that fixes a bug.

If the there is a test failing before the fix it would be awesome since it show that it is indeed a bug :)

Interesting.... I would have thought that go would have captured m and dereferenced it. But I guess it makes sense to capture m.tracked directly

ArthurSens

I wonder if we couldn't use componentTestTelemetry instead? Testing telemetry is something common to all components in the collector, sounds strange that we need to implement our own comparison logic

opentelemetry-collector-contrib/processor/deltatocumulativeprocessor/generated_component_telemetry_test.go

Lines 45 to 56 in 08a86b4

    
           func (tt *componentTestTelemetry) assertMetrics(t *testing.T, expected []metricdata.Metrics) { 
        
           	var md metricdata.ResourceMetrics 
        
           	require.NoError(t, tt.reader.Collect(context.Background(), &md)) 
        
           	// ensure all required metrics are present 
        
           	for _, want := range expected { 
        
           		got := tt.getMetric(want.Name, md) 
        
           		metricdatatest.AssertEqual(t, want, got, metricdatatest.IgnoreTimestamp()) 
        
           	} 
        
           	// ensure no additional metrics are emitted 
        
           	require.Equal(t, len(expected), tt.len(md)) 
        
           }

processor/deltatocumulativeprocessor/internal/testing/sdktest/example_test.go

ArthurSens · 2024-11-01T17:13:35Z

diff --git a/processor/deltatocumulativeprocessor/processor_test.go b/processor/deltatocumulativeprocessor/processor_test.go
index 12d4452e62..cfc28dc617 100644
--- a/processor/deltatocumulativeprocessor/processor_test.go
+++ b/processor/deltatocumulativeprocessor/processor_test.go
@@ -14,11 +14,13 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/collector/config/configtelemetry"
 	"go.opentelemetry.io/collector/confmap/confmaptest"
 	"go.opentelemetry.io/collector/consumer/consumertest"
 	"go.opentelemetry.io/collector/pdata/pmetric"
 	"go.opentelemetry.io/collector/processor"
-	"go.opentelemetry.io/collector/processor/processortest"
+	"go.opentelemetry.io/otel/metric"
+	"go.opentelemetry.io/otel/metric/noop"
 	"go.opentelemetry.io/otel/sdk/metric/metricdata"
 	"gopkg.in/yaml.v3"
 
@@ -54,7 +56,7 @@ func TestProcessor(t *testing.T) {
 
 			ctx := context.Background()
 			cfg := config(t, file("config.yaml"))
-			proc, sink := setup(t, cfg)
+			proc, sink, tel := setup(t, cfg)
 
 			stages, _ := filepath.Glob(file("*.test"))
 			for _, file := range stages {
@@ -71,6 +73,8 @@ func TestProcessor(t *testing.T) {
 					t.Fatal(diff)
 				}
 			}
+
+			tel.assertMetrics(t, metrics)
 		})
 
 	}
@@ -89,7 +93,7 @@ func config(t *testing.T, file string) *Config {
 	return cfg
 }
 
-func setup(t *testing.T, cfg *Config) (processor.Metrics, *consumertest.MetricsSink) {
+func setup(t *testing.T, cfg *Config) (processor.Metrics, *consumertest.MetricsSink, componentTestTelemetry) {
 	t.Helper()
 
 	next := &consumertest.MetricsSink{}
@@ -97,15 +101,27 @@ func setup(t *testing.T, cfg *Config) (processor.Metrics, *consumertest.MetricsS
 		cfg = &Config{MaxStale: 0, MaxStreams: math.MaxInt}
 	}
 
+	tel := setupTestTelemetry()
+	set := tel.NewSettings()
+	mp := set.LeveledMeterProvider(configtelemetry.LevelBasic)
+	set.LeveledMeterProvider = func(level configtelemetry.Level) metric.MeterProvider {
+		// detailed level enables otelhttp client instrumentation which we
+		// dont want to test here
+		if level == configtelemetry.LevelDetailed {
+			return noop.MeterProvider{}
+		}
+		return mp
+	}
+
 	proc, err := NewFactory().CreateMetrics(
 		context.Background(),
-		processortest.NewNopSettings(),
+		set,
 		cfg,
 		next,
 	)
 	require.NoError(t, err)
 
-	return proc, next
+	return proc, next, tel
 }

sh0rez · 2024-11-01T18:01:08Z

@ArthurSens I considered assertMetrics and felt it ultimately does not do what we need.

First of all, it requires you to specify all metrics emitted by the reader, which we rarely want. We only want to test the metrics relevant to the specific test case.

Second, you need some way to get that expected slice, and the most of sdktest implements exactly that: a format to specify the expected slice.

For actual comparison, it just calls out to go-cmp which imo yields way more usable results that assertMetrics does:

assertMetrics:

 processor_test.go:80: [Metrics Data not equal: Sum[int64] not equal: Sum DataPoints not equal:
     missing expected values:
     metricdata.DataPoint[int64]{Attributes:attribute.Set{equivalent:attribute.Distinct{iface:[1]attribute.KeyValue{attribute.KeyValue{Key:"error", Value:attribute.Value{vtype:4, numeric:0x0, stringly:"delta.ErrOlderStart", slice:interface {}(nil)}}}}}, StartTime:time.Date(2024, time.November, 1, 18, 54, 31, 646527291, time.Local), Time:time.Date(2024, time.November, 1, 18, 54, 31, 647399262, time.Local), Value:2, Exemplars:[]metricdata.Exemplar[int64]{}}
     unexpected additional values:
     metricdata.DataPoint[int64]{Attributes:attribute.Set{equivalent:attribute.Distinct{iface:[1]attribute.KeyValue{attribute.KeyValue{Key:"error", Value:attribute.Value{vtype:4, numeric:0x0, stringly:"delta.ErrOlderStart", slice:interface {}(nil)}}}}}, StartTime:time.Date(2024, time.November, 1, 18, 54, 31, 646527291, time.Local), Time:time.Date(2024, time.November, 1, 18, 54, 31, 647421097, time.Local), Value:1, Exemplars:[]metricdata.Exemplar[int64]{}}
     ]

sdktest:

 processor_test.go:83:
       []metricdata.Metrics{
             {
                     Name:        "otelcol_deltatocumulative.datapoints.linear",
                     Description: "total number of datapoints processed. may have 'error' attribute"...,
                     Unit:        "{datapoint}",
                     Data: metricdata.Sum[int64]{
                             DataPoints: []metricdata.DataPoint[int64](Inverse(sdktest.Transform.int64, []sdktest.DataPoint[int64]{
                                     {DataPoint: {StartTime: s"2024-11-01 18:54:31.646527291 +0100 CET m=+0.020564831", Time: s"2024-11-01 18:54:31.647621443 +0100 CET m=+0.021658982", Value: 3, Exemplars: {}, ...}, Attributes: {}},
                                     {
                                             DataPoint: metricdata.DataPoint[int64]{
                                                     ... // 1 ignored field
                                                     StartTime: s"2024-11-01 18:54:31.646527291 +0100 CET m=+0.020564831",
                                                     Time:      s"2024-11-01 18:54:31.647621443 +0100 CET m=+0.021658982",
     -                                               Value:     2,
     +                                               Value:     1,
                                                     Exemplars: {},
                                             },
                                             Attributes: {"error": string("delta.ErrOlderStart")},
                                     },
                                     {DataPoint: {StartTime: s"2024-11-01 18:54:31.646527291 +0100 CET m=+0.020564831", Time: s"2024-11-01 18:54:31.647621443 +0100 CET m=+0.021658982", Value: 1, Exemplars: {}, ...}, Attributes: {"error": string("delta.ErrOutOfOrder")}},
                             })),
                             Temporality: s"CumulativeTemporality",
                             IsMonotonic: true,
                     },
             },
       }

I can honestly not even spot the difference in the assertMetrics output (without knowing from the sdktest diff below)

Taking a closer look, I think I can cut down the amount of work sdktest does by relying more on go-cmp ignorers ... similar to how assertMetrics is implemented with those Ignore* options

ArthurSens · 2024-11-01T18:28:07Z

First of all, it requires you to specify all metrics emitted by the reader, which we rarely want. We only want to test the metrics relevant to the specific test case.

Could we contribute this?

I'd rather stick to what everybody is using, and improve things for everybody if we see limitations. I don't see why people would refuse better comparison output :)

Tests internal telemetry (metadata.TelemetryBuilder) is recorded as expected. Introduces `internal/testing/sdktest` for this. Introduces `-- telemetry --` section to testdata.

All of the partial diffing we do by manually copying values can be achieved using go-cmp Filters and Transformers. This a lot cleaner and less maintenance

due to a bad closure capture, the callback registration never happened

ArthurSens

Hey @RichieSams, I am just summarizing here what @sh0rez and I discussed in Slack:

Although we're writing a lot of code here that concerns our test framework, the PR does test that the telemetry behaves as expected, so let's merge this to unblock other work.

We did agree that we don't appreciate maintaining a full-blown test framework just for us. It's extra work for the maintainers of this component and we don't help the whole collector community evolve what they have.

Task for the future:

Propose better test output for collector-core's testing framework.

MovieStoreGuy · 2024-11-14T04:51:08Z

processor/deltatocumulativeprocessor/testdata/exponential/1.test

Any reason why this isn't a .yml or .yaml file?

it's technically not a yaml file.

this uses txtar to bundle several yaml files (in, out, telemetry) into one physical file. This is nice because:

input and output in the same file

multiple write-requests per testcase as each request is a .test file. This allows to test processor state across write requests

the .test ending is arbitrary but I thought it serves the purpose here

jpkrohling · 2024-11-19T11:03:24Z

processor/deltatocumulativeprocessor/internal/lineartelemetry/metrics.go

@@ -16,12 +16,13 @@ import (
 )

 func New(set component.TelemetrySettings) (Metrics, error) {
+	zero := func() int { return -1 }


Am I the only one who thinks "zero" is a bad name for something that returns -1? Also because the previous iteration of the code returned zero...

ugh this is an oversight.
I will address this in a later PR

#### Description Tests internal telemetry (metadata.TelemetryBuilder) is recorded as expected. Introduces `internal/testing/sdktest` for this. Introduces `-- telemetry --` section to testdata.  #### Testing Existing tests were extended to have a `-- telemetry --` section that specifies expected meter readings in `sdktest.Format`  #### Documentation not needed

sh0rez requested review from jpkrohling and a team as code owners October 11, 2024 12:49

github-actions bot assigned MovieStoreGuy Oct 11, 2024

github-actions bot added the processor/deltatocumulative label Oct 11, 2024

github-actions bot requested a review from RichieSams October 11, 2024 12:50

sh0rez marked this pull request as draft October 11, 2024 12:50

sh0rez force-pushed the deltatocumulative-teltest branch from 684beda to 2dc5560 Compare October 18, 2024 15:47

sh0rez marked this pull request as ready for review October 18, 2024 15:49

github-actions bot assigned djaglowski Oct 18, 2024

sh0rez commented Oct 18, 2024

View reviewed changes

sh0rez force-pushed the deltatocumulative-teltest branch 2 times, most recently from e57fcf4 to c379bbd Compare November 1, 2024 13:19

ArthurSens reviewed Nov 1, 2024

View reviewed changes

processor/deltatocumulativeprocessor/internal/testing/sdktest/example_test.go Show resolved Hide resolved

sh0rez added 2 commits November 13, 2024 13:58

[processor/deltatocumulative]: telemetry tests

ecbb1ad

Tests internal telemetry (metadata.TelemetryBuilder) is recorded as expected. Introduces `internal/testing/sdktest` for this. Introduces `-- telemetry --` section to testdata.

*: test histograms, exponential

e39f959

sh0rez force-pushed the deltatocumulative-teltest branch from bfd99b0 to d381aa2 Compare November 13, 2024 12:59

sh0rez added 2 commits November 13, 2024 14:15

sdktest: simplify using go-cmp

5a0f82c

All of the partial diffing we do by manually copying values can be achieved using go-cmp Filters and Transformers. This a lot cleaner and less maintenance

sdktest: godoc

97af96b

sh0rez force-pushed the deltatocumulative-teltest branch from d381aa2 to 97af96b Compare November 13, 2024 13:15

lineartelemetry: properly register callback

0bfec5b

due to a bad closure capture, the callback registration never happened

ArthurSens approved these changes Nov 13, 2024

View reviewed changes

MovieStoreGuy reviewed Nov 14, 2024

View reviewed changes

jpkrohling added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Nov 19, 2024

jpkrohling approved these changes Nov 19, 2024

View reviewed changes

jpkrohling merged commit 9466fb2 into open-telemetry:main Nov 19, 2024
167 of 168 checks passed

github-actions bot added this to the next release milestone Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

processor/deltatocumulative: telemetry tests #35742

processor/deltatocumulative: telemetry tests #35742

sh0rez commented Oct 11, 2024 •

edited

Loading

sh0rez Oct 18, 2024

RichieSams Oct 21, 2024

sh0rez Nov 1, 2024

ArthurSens Nov 1, 2024

sh0rez Nov 1, 2024

RichieSams Nov 5, 2024 •

edited

Loading

ArthurSens left a comment

ArthurSens commented Nov 1, 2024

sh0rez commented Nov 1, 2024 •

edited

Loading

ArthurSens commented Nov 1, 2024 •

edited

Loading

ArthurSens left a comment

MovieStoreGuy Nov 14, 2024

sh0rez Nov 14, 2024

jpkrohling Nov 19, 2024

sh0rez Nov 19, 2024

	m := Metrics{
	tracked: &zero,
	}

	trackedCb := metadata.WithDeltatocumulativeStreamsTrackedLinearCallback(func() int64 {
	return int64((*m.tracked)())
	})

	func (tt componentTestTelemetry) assertMetrics(t testing.T, expected []metricdata.Metrics) {
	var md metricdata.ResourceMetrics
	require.NoError(t, tt.reader.Collect(context.Background(), &md))
	// ensure all required metrics are present
	for _, want := range expected {
	got := tt.getMetric(want.Name, md)
	metricdatatest.AssertEqual(t, want, got, metricdatatest.IgnoreTimestamp())
	}

	// ensure no additional metrics are emitted
	require.Equal(t, len(expected), tt.len(md))
	}

processor/deltatocumulative: telemetry tests #35742

processor/deltatocumulative: telemetry tests #35742

Conversation

sh0rez commented Oct 11, 2024 • edited Loading

Description

Testing

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichieSams Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens commented Nov 1, 2024

sh0rez commented Nov 1, 2024 • edited Loading

ArthurSens commented Nov 1, 2024 • edited Loading

ArthurSens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sh0rez commented Oct 11, 2024 •

edited

Loading

RichieSams Nov 5, 2024 •

edited

Loading

sh0rez commented Nov 1, 2024 •

edited

Loading

ArthurSens commented Nov 1, 2024 •

edited

Loading