Skip to content

Conversation

@aboguszewski-sumo
Copy link
Member

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.

The capacity of the queue can now be expressed in bytes. Because of that, parameter queue_size has been renamed to queue_size_batches.
@aboguszewski-sumo aboguszewski-sumo requested review from a team and Aneurysm9 July 27, 2022 16:46
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 27, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: aboguszewski-sumo / name: Adam Boguszewski (c1598a9, 3e67a59)

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

We declared the configuration for this stable, and is probably one of the most used config, so cannot break it.

@aboguszewski-sumo
Copy link
Member Author

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.

@bogdandrutu
Copy link
Member

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?

@aboguszewski-sumo
Copy link
Member Author

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: [exporterhelper] enable Persistent Queue feature by default #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

@bogdandrutu
Copy link
Member

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

@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Aug 20, 2022
@aboguszewski-sumo
Copy link
Member Author

aboguszewski-sumo commented Aug 25, 2022

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.

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
* Update core/contrib deps

* Update renamed components

* Update signalfx exporter for contrib open-telemetry#5756

* Update CHANGELOG.md

* Fix breaking config map and tracer changes

* Account for k8s_tagger rename

* Rename k8s_tagger to k8sattributes

* reduce collectd-cassandra test flake

* improve k8s_tagger rename test

* Update hec tls config map usage

* move from retracted 0.37.0 to 0.37.1

* Update changelog

* Update bundled Smart Agent to v5.14.2

* Update github.com/signalfx/signalfx-agent to main

Co-authored-by: Ryan Fitzpatrick <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants