Skip to content

Update OTel-Arrow exporter to use QueueBatch settings #40211

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 6, 2025

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented May 21, 2025

Description

Following the completion of open-telemetry/opentelemetry-collector#8122.

Link to tracking issue

open-telemetry/otel-arrow#240

Testing

Documentation

@jmacd
Copy link
Contributor Author

jmacd commented May 21, 2025

Let's assume I can figure out the problem with the lifecycle tests introduced by switching defaults here, however, this test failure confuses me:

=== RUN   TestCreateDefaultConfig
    factory_test.go:37: 
        	Error Trace:	/home/jmacd/src/otel/collector-contrib/exporter/otelarrowexporter/factory_test.go:37
        	Error:      	Not equal: 
        	            	expected: queuebatch.Config{Enabled:true, WaitForResult:true, Sizer:request.SizerType{val:"items"}, QueueSize:1000, BlockOnOverflow:true, Blocking:false, StorageID:(*component.ID)(nil), NumConsumers:110, Batch:(*queuebatch.BatchConfig)(0xc0001d8348), hasBlocking:false}
        	            	actual  : queuebatch.Config{Enabled:true, WaitForResult:false, Sizer:request.SizerType{val:"requests"}, QueueSize:1000, BlockOnOverflow:false, Blocking:false, StorageID:(*component.ID)(nil), NumConsumers:10, Batch:(*queuebatch.BatchConfig)(nil), hasBlocking:false}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -2,16 +2,12 @@
        	            	  Enabled: (bool) true,
        	            	- WaitForResult: (bool) true,
        	            	+ WaitForResult: (bool) false,
        	            	  Sizer: (request.SizerType) {
        	            	-  val: (string) (len=5) "items"
        	            	+  val: (string) (len=8) "requests"
        	            	  },
        	            	  QueueSize: (int64) 1000,
        	            	- BlockOnOverflow: (bool) true,
        	            	+ BlockOnOverflow: (bool) false,
        	            	  Blocking: (bool) false,
        	            	  StorageID: (*component.ID)(<nil>),
        	            	- NumConsumers: (int) 110,
        	            	- Batch: (*queuebatch.BatchConfig)({
        	            	-  FlushTimeout: (time.Duration) 1000000000,
        	            	-  MinSize: (int64) 1000,
        	            	-  MaxSize: (int64) 1500
        	            	- }),
        	            	+ NumConsumers: (int) 10,
        	            	+ Batch: (*queuebatch.BatchConfig)(<nil>),
        	            	  hasBlocking: (bool) false
        	Test:       	TestCreateDefaultConfig

Context: the OTel-Arrow exporter documentation has always referred to the concurrentbatchprocessor which was maintained in the otel-arrow repository and recently removed because we consider the exporterhelper support "Ready". However, I tried to change the default settings to enable two boolean flags and batching by default w/ the concurrentbatchprocessor defaults (i.e., 1s timeout, 1000-1500 item batches). I made these changes in the default config function, but they don't stick and the test fails comparing a parsed default config with the original default config.

See https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/40211/files#r2101162598.

@mx-psi
Copy link
Member

mx-psi commented May 23, 2025

I made these changes in the default config function, but they don't stick and the test fails comparing a parsed default config with the original default config.

If I am looking at the right test this is not comparing a parsed default to the original default: it's comparing the result of the CreateDefaultConfig factory method with a hand-build struct. In particular this fails:

assert.Equal(t, ocfg.QueueSettings, exporterhelper.NewDefaultQueueConfig())

because of these changes added on the PR

queueCfg.WaitForResult = true
queueCfg.BlockOnOverflow = true
queueCfg.Sizer = exporterhelper.RequestSizerTypeItems
queueCfg.NumConsumers = arrow.DefaultProducersPerStream * arrow.DefaultNumStreams
queueCfg.Batch = &exporterhelper.BatchConfig{
FlushTimeout: time.Second,
MinSize: 1000,
MaxSize: 1500,
}

I think the test you are thinking about is TestUnmarshalDefaultConfig which does pass locally on my machine:

Running tool: /home/pablo.baeyens/.gvm/gos/go1.23.4/bin/go test -timeout 30s -tags cgo,otlp,!python,test -run ^TestUnmarshalDefaultConfig$ github.com/open-telemetry/opentelemetry-collector-contrib/receiver/otelarrowreceiver

ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/otelarrowreceiver	0.015s

@jmacd
Copy link
Contributor Author

jmacd commented May 23, 2025

I was looking at the wrong test. Sorry, that was obvious!
I'm looking into the actual failures now 😊

@jmacd
Copy link
Contributor Author

jmacd commented May 23, 2025

@jmacd
Copy link
Contributor Author

jmacd commented May 23, 2025

I ran into trouble with WaitForResult: true (see link above) and decided not to push for that default. I've adjusted the default parameters to be appropriate IMO for Arrow to achieve good throughput in a gateway configuration, but still not huge.

@jmacd jmacd marked this pull request as ready for review May 23, 2025 20:14
@jmacd jmacd requested a review from a team as a code owner May 23, 2025 20:14
@jmacd jmacd requested a review from crobert-1 May 23, 2025 20:14
Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine to me, just one thing about the README.md change

Copy link

@lquerel lquerel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mx-psi mx-psi merged commit d7583cc into open-telemetry:main Jun 6, 2025
177 checks passed
@github-actions github-actions bot added this to the next release milestone Jun 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants