Skip to content
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

feat: send drop decisions in batch #1402

Merged
merged 4 commits into from
Oct 31, 2024
Merged

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented Oct 29, 2024

Which problem is this PR solving?

To reduce network traffic from nodes communicating about trace decisions, drop decisions will be sent in batches. The batching logic is based on either a maximum batch size or a timer. Refinery will send a batch when the batch size limit is reached or when the timer expires, whichever comes first.

Short description of the changes

  • add sendDropDecisions goroutine
  • add two new config options for the batch size and the ticker interval. This is mostly for easier testing. We may not include these two configs in the final release.

@VinozzZ VinozzZ added the type: enhancement New feature or request label Oct 29, 2024
@VinozzZ VinozzZ added this to the v2.9 milestone Oct 29, 2024
@VinozzZ VinozzZ self-assigned this Oct 29, 2024
@VinozzZ VinozzZ requested a review from a team as a code owner October 29, 2024 16:06
@VinozzZ VinozzZ changed the title Yingrong/batch drop decisions feat: send drop decisions in batch Oct 29, 2024
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

I'm happy with this, but left one comment to think about.

if len(traceIDs) >= i.Config.GetCollectionConfig().MaxDropDecisionBatchSize {
send = true
}
case <-ticker.Chan():
Copy link
Contributor

Choose a reason for hiding this comment

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

This gives us a possible situation like this:

  • Max size is reached at time T
  • We send the batch
  • We get one decision
  • At T+1ms the ticker fires
  • We send that batch

If the number of decisions per second is close to the max batch size, then the above is very likely to happen.

I've thought of two possible ways to deal with this:

  • Keep a timestamp of the oldest item in the batch; if it's less than our ticker time, then don't send yet.
  • Don't use a ticker, use a timer, and restart the timer after every send, no matter why we sent it.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the timer idea. I will update the code to do that

@VinozzZ VinozzZ merged commit 01084b8 into main Oct 31, 2024
5 checks passed
@VinozzZ VinozzZ deleted the yingrong/batch_drop_decisions branch October 31, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants