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

appender.go

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?

Related to #249 (comment), I'm not sure how the doc size is calculated, but increasing this allows the tests to pass. cc @kruskall since he added this test I believe. If it stays at 750, it triggers a flush on 9 docs on my Mac, but 8 in CI, causing the metrics below to be incorrect.

appender_test.go

appender.go

appender_test.go

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?

Good Q. I should maybe add some comments, since this took me some time to figure out. I initially tested this by checking the actual values produced by Stats. Then I made sure the number makes sense. Basically, the test sends 10 docs. 3 of them failing, hence the first Success above is 7. Then a retry happens on TooMany that should succeed, which leads to a Success of 8

appender_test.go

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?

Addressed in #249 (comment)

Additionally, a lot of the tests check the metrics before and after some event/condition. This originated from using Stats in which one reads absolute values (Cumulative in OTEL) and then do a comparison. However, we have an opportunity here to leverage DeltaTemporality and simplifying the test structures. But this will require some deeper thought and will really blow up this PR's diff. So I was thinking of creating a new issue to address this is a separate PR in the next iter once this one has been merged. cc @raultorrecilla

appender_test.go

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

appender.go Outdated Show resolved Hide resolved

simitt reviewed May 28, 2025

View reviewed changes

rubvs added 2 commits May 29, 2025 10:28

fix toomany metric update

c4b336a

update error assert to require

babc31b

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!

Uh oh!

simitt left a comment

Choose a reason for hiding this comment

Uh oh!

simitt May 28, 2025

Choose a reason for hiding this comment

Uh oh!

rubvs May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simitt May 28, 2025

Choose a reason for hiding this comment

Uh oh!

rubvs May 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simitt May 28, 2025

Choose a reason for hiding this comment

Uh oh!

rubvs May 29, 2025

Choose a reason for hiding this comment

Uh oh!

rubvs May 29, 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!

Uh oh!

Uh oh!

rubvs commented May 2, 2025 •

edited

Loading

rubvs commented May 16, 2025 •

edited

Loading

rubvs May 23, 2025 •

edited

Loading

rubvs May 29, 2025 •

edited

Loading