opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

[exporter/exporterhelper] allow to specify max queue size in bytes

Open aboguszewski-sumo opened this issue 3 years ago • 5 comments

Description: This PR resolves a problem described more precisely in the linked issue. TL;DR: The persistent queue now supports specifying an upper limit for the queue capacity in bytes. The queue_size parameter is now deprecated and serves as an alias for queue_size_batches.

Link to tracking Issue: Resolves #5213

Testing: Some of the old tests have been modified insignificantly, by adding a bonus parameter to some functions etc. The main functionality added in this PR has been tested with a test very similar to the test for the capacity specified in batches.

aboguszewski-sumo avatar Jul 27 '22 16:07 aboguszewski-sumo

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: aboguszewski-sumo / name: Adam Boguszewski (c1598a9eeaf813c7310b647f7dc6c59953a5928a, 3e67a59ae6e286e1219271190dbbef9e206b0680)

Do you mean the change from queue_size to queue_size_batches? It's fine, I can revert it since it's not a must-have change in this case. I don't think it's a breaking change though, after these changes if queue_size has been specified in the config, a warning is logged and that value is used instead of queue_size_batches so that the old builds won't break.

aboguszewski-sumo avatar Jul 28 '22 08:07 aboguszewski-sumo

I don't think it's a breaking change though, after these changes if queue_size has been specified in the config, a warning is logged and that value is used instead of queue_size_batches so that the old builds won't break.

What is the motivation to have all this logic? Why is not the storage extension configured to have a size in bytes instead of the user?

bogdandrutu avatar Jul 28 '22 18:07 bogdandrutu

I am not sure I understand what's the problem, so I will elaborate on what this PR is about:

  • The main change (specifying the queue size in bytes) affects only the persistent queue, which is experimental at the moment (but there are plans to make it stable: #5711).
  • Limiting the queue size in bytes directly in the queue config seems more intuitive. It is already possible to specify the limit in batches here, so it's logical to put other similar limits here. Moreover, the storage as the place to configure this does not sound intuitive at all - the limit of the queue is a thing that is related directly to the queue, and storage is just a tool used to implement the queue.
  • The deprecation of queue_size in favor of queue_size_batches was only a suggestion from the issue and is not necessary.

cc: @swiatekm-sumo

aboguszewski-sumo avatar Jul 29 '22 10:07 aboguszewski-sumo

This PR needs to be re-created after the cleanup of enabling the persistent queue.

bogdandrutu avatar Aug 05 '22 17:08 bogdandrutu

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Aug 20 '22 03:08 github-actions[bot]

I've recreated this PR on another branch, but it seems that there are some things that need to be changed in the persistent queue before this feature can be added seamlessly. I'm closing this for now and will create another PR when it will be time for it.

aboguszewski-sumo avatar Aug 25 '22 14:08 aboguszewski-sumo