Remove Stats datastructure in favour of OTel #249

rubvs · 2025-05-02T18:56:56Z

Closes #246

Remove the Stats datastructure and leverage the current OTel implementation.

Updated Unit Tests

appender.go

appender_test.go

appender.go

1pkg

Overall the changes looks good, thank you!

Albeit OTel metrics assertions are quite verbose and repetitive, it would be nice to sugar it up a bit to reduce the tests code. I would recommend to look into expanding docappendertest.AssertOTelMetrics helper to accept a metrics assertion map.

docappendertest.AssertOTelMetricsWithMap(t, rm.ScopeMetrics[0].Metrics, map[string]int64{
	"elasticsearch.bulk_requests.count": 1,
	"elasticsearch.indexer.created": 2,
	"elasticsearch.indexer.destroyed": 3,
})

With such helper most if not all of the tests could be simmered down a bit.

appender_test.go

rubvs · 2025-05-16T21:08:06Z

Finally the CI passes. This TestAppenderRetry needs to be rethought. How did we get to 750 here, see Code? cc @kruskall

rubvs · 2025-05-23T14:42:25Z

appender_test.go

 	const N = 10
 	for i := 0; i < N; i++ {
 		addMinimalDoc(t, indexer, "logs-foo-testing")
 	}
+	<-time.After(1 * time.Second)


For Reviewers: I have created this ticket to improve this wait using a channel: #253

kruskall · 2025-05-28T13:30:21Z

appender.go

 			}
 			if status != "" {
-				a.addCount(int64(n), legacy, a.metrics.docsIndexed,
-					metric.WithAttributes(attribute.String("status", status), semconv.HTTPResponseStatusCode(errFailed.statusCode)),
+				a.tooManyRequests.Add(legacy)


issue: I think this is updating the wrong metric. It should only update toomanyrequests in the switch case for tooMany

simitt

Left a bunch of smaller questions in the first review.

simitt · 2025-05-28T06:50:14Z

appender_test.go

@@ -274,7 +268,7 @@ func TestAppenderRetry(t *testing.T) {

 	indexer, err := docappender.New(client, docappender.Config{
 		FlushInterval:      time.Minute,
-		FlushBytes:         750, // this is enough to flush after 9 documents
+		FlushBytes:         800, // this is enough to flush after 9 documents


Why does this value change?

simitt · 2025-05-28T06:50:42Z

appender_test.go

@@ -288,79 +282,74 @@ func TestAppenderRetry(t *testing.T) {
 	for i := 0; i < N; i++ {
 		addMinimalDoc(t, indexer, "logs-foo-testing")
 	}
+	<-time.After(1 * time.Second)


Is 1 second enough? it used to be 2 seconds..

simitt · 2025-05-28T13:44:33Z

appender.go

 			}
 			if status != "" {
-				a.addCount(int64(n), legacy, a.metrics.docsIndexed,
-					metric.WithAttributes(attribute.String("status", status), semconv.HTTPResponseStatusCode(errFailed.statusCode)),
+				a.tooManyRequests.Add(legacy)


Would it make sense to add this in the errFailed.tooMany case?

simitt · 2025-05-28T13:52:48Z

appender_test.go

+
+	// Collect metrics before flushing.
+	var rm metricdata.ResourceMetrics
+	assert.NoError(t, rdr.Collect(context.Background(), &rm))


Suggested change

assert.NoError(t, rdr.Collect(context.Background(), &rm))

require.NoError(t, rdr.Collect(context.Background(), &rm))

simitt · 2025-05-28T13:57:24Z

appender_test.go

+		case "elasticsearch.events.count":
+			assertCounter(m, N, indexerAttrs)
+		case "elasticsearch.events.queued":
+			assertCounter(m, N, indexerAttrs)


So far this test tested for docs.active - why is this not necessary anymore?

simitt · 2025-05-28T14:03:48Z

appender_test.go

+				assert.True(t, exist)
+				switch status.AsString() {
+				case "Success":
+					assert.Equal(t, int64(8), dp.Value)


Where is this number from?

simitt · 2025-05-28T14:04:22Z

appender_test.go

-		BulkRequests:           2,
-		Failed:                 failed,
-		FailedClient:           1,
-		FailedServer:           1,


Where are these checked now?

simitt · 2025-05-28T14:05:46Z

appender_test.go

@@ -92,7 +92,7 @@ func TestAppender(t *testing.T) {

 	rdr := sdkmetric.NewManualReader(sdkmetric.WithTemporalitySelector(
 		func(ik sdkmetric.InstrumentKind) metricdata.Temporality {
-			return metricdata.DeltaTemporality
+			return metricdata.CumulativeTemporality


Why does this change to Cumulative?

simitt · 2025-05-28T14:06:36Z

appender_test.go

-		BulkRequests:           1,
-		Failed:                 failed,
-		FailedClient:           1,
-		FailedServer:           1,


Where are the failed events checked now? Haven't seen them below

remove stats struct and update unit tests

33254f1

elastic-observability-automation bot added the safe-to-test Automated label for running bench-diff on forked PRs label May 2, 2025

update TestAppenderScaling

ec67b0f

rubvs marked this pull request as ready for review May 5, 2025 17:43

rubvs requested a review from a team as a code owner May 5, 2025 17:43

rubvs requested a review from kruskall May 5, 2025 17:44

rubvs force-pushed the remove-stats-datastructure branch 4 times, most recently from 525b8b3 to 1141f1c Compare May 5, 2025 18:22

ci: fix unit test failing in CI

c6f2f06

rubvs force-pushed the remove-stats-datastructure branch from 1141f1c to c6f2f06 Compare May 5, 2025 18:36

kruskall reviewed May 5, 2025

View reviewed changes

appender.go Outdated Show resolved Hide resolved

appender_test.go Outdated Show resolved Hide resolved

appender_test.go Show resolved Hide resolved

appender.go Outdated Show resolved Hide resolved

1pkg reviewed May 5, 2025

View reviewed changes

appender.go Outdated Show resolved Hide resolved

1pkg previously approved these changes May 5, 2025

View reviewed changes

kruskall requested changes May 8, 2025

View reviewed changes

appender_test.go Show resolved Hide resolved

re-introduce function locality

59c12d9

rubvs dismissed 1pkg’s stale review via 59c12d9 May 11, 2025 14:13

rubvs marked this pull request as draft May 11, 2025 18:52

rubvs force-pushed the remove-stats-datastructure branch 7 times, most recently from b450780 to 89c79b2 Compare May 11, 2025 23:19

fix CI failure

e650c89

rubvs force-pushed the remove-stats-datastructure branch from 89c79b2 to e650c89 Compare May 16, 2025 21:04

remove reduntant addCount method

4e97163

remove reduntant addUpDownCount method

e9e3384

rubvs force-pushed the remove-stats-datastructure branch from e2de81c to f48cdad Compare May 22, 2025 20:37

revert formating

bd8e118

rubvs force-pushed the remove-stats-datastructure branch from f48cdad to bd8e118 Compare May 22, 2025 20:47

rubvs marked this pull request as ready for review May 23, 2025 02:38

rubvs requested review from kruskall and 1pkg May 23, 2025 14:18

revert some otel reader temporality

fd439fa

rubvs commented May 23, 2025

View reviewed changes

rubvs mentioned this pull request May 27, 2025

Add function hooks to config #253

Open

kruskall reviewed May 28, 2025

View reviewed changes

simitt reviewed May 28, 2025

View reviewed changes

	assert.NoError(t, rdr.Collect(context.Background(), &rm))
	require.NoError(t, rdr.Collect(context.Background(), &rm))

Remove Stats datastructure in favour of OTel #249

Are you sure you want to change the base?

Remove Stats datastructure in favour of OTel #249

Conversation

rubvs commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Updated Unit Tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

1pkg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rubvs commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rubvs May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rubvs commented May 2, 2025 •

edited

Loading

rubvs commented May 16, 2025 •

edited

Loading

rubvs May 23, 2025 •

edited

Loading