Skip to content

Conversation

@kwvg
Copy link

@kwvg kwvg commented Mar 9, 2025

Expected behavior

  • Create StatsdClient with batchsize
  • Push a few messages that are under batchsize, remains in same batch
  • Push a message that would go over batchsize if appended, gets put into a new batch
  • Trigger flush, two batches are created, both under batchsize

Actual behavior

  • Create StatsdClient with batchsize
  • Push a few messages that are under batchsize, remains in same batch
  • Push a message that would go over batchsize if appended, gets put into same batch
  • Flush triggered, one batch is created exceeding batchsize

Steps to reproduce

  • Revert e3bffa3 and run tests, a hyphen (-) is used to indicate a batch

@kevinkreiser
Copy link
Collaborator

@kwvg the documentation https://github.com/vthiery/cpp-statsd-client?tab=readme-ov-file#batching explicitly states that the batch size is not a hard limit (indeed it cant be without quite a bit of annoyance, eg. in the case where the batch size is so small a single message wouldnt fit). so i wouldnt characterize this change as a "fix" in this case. this is merely just more of an attempt to make batch size a hard limit right? but even still, the change here does not make it a hard limit specifically in the example i mentioned above. i also see that you keep the additional memory reservation when adding a new batch, which makes no sense if we are trying to stick to a hard limit.

at any rate, i think we should first discuss the merits of a hard limit vs the practical reality. perhaps we can throw or something if someone wants to make a batch size that is too small to be reasonable so as to avoid the pitfall i mentioned above? at present i dont feel strong about having a hard limit vs a soft limit. the reason why we went with a soft one though in the first place was simply because it was just quite a bit less difficult to adhere to logically 😄

// Either we don't have a place to batch our message or we are about to exceed the batch size, so make a new batch
if (m_batchingMessageQueue.empty() || m_batchingMessageQueue.back().size() + message.size() > m_batchsize) {
m_batchingMessageQueue.emplace_back();
m_batchingMessageQueue.back().reserve(m_batchsize + 256);
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we really do want a hard limit we should add some fuzz to the batch size for overages:

Suggested change
m_batchingMessageQueue.back().reserve(m_batchsize + 256);
m_batchingMessageQueue.back().reserve(m_batchsize);

@kevinkreiser
Copy link
Collaborator

linting step is broken we'll have to fix that on our end. you can ignore that for now 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants