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: batch kept decisions #1419

Merged
merged 6 commits into from
Nov 8, 2024
Merged

feat: batch kept decisions #1419

merged 6 commits into from
Nov 8, 2024

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented Nov 8, 2024

Which problem is this PR solving?

The throughput of Redis pub/sub is impacted by the number of messages sent per time unit. To improve performance and ensure a higher throughput of kept messages, we need to reduce the number of kept messages sent.
This PR implements a batching strategy similar to the one used for dropped messages, reducing the frequency of sending individual kept messages.

Short description of the changes

  • Introduced a batching strategy for kept messages, similar to the existing strategy for dropped messages.
  • Refactor kept and dropped decision processing to reduce duplicated code

@VinozzZ VinozzZ requested a review from a team as a code owner November 8, 2024 18:38
@VinozzZ VinozzZ self-assigned this Nov 8, 2024
@VinozzZ VinozzZ added the type: enhancement New feature or request label Nov 8, 2024
@VinozzZ VinozzZ added this to the v2.9 milestone Nov 8, 2024
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Generally looks great, left a couple of comments.

collect/collect.go Outdated Show resolved Hide resolved
if trace == nil {
i.Logger.Debug().Logf("trace not found in cache for trace decision")
return
// Send the processed trace (only in Kept case)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can consolidate these two if blocks together.

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 realized that we should still call send for dropped traces since the trace_send_dropped metric is in there and it does check against trace.Kept inside send anyway

@VinozzZ VinozzZ merged commit 20896ee into main Nov 8, 2024
5 checks passed
@VinozzZ VinozzZ deleted the yingrong/batch_kept_decisions branch November 8, 2024 21:20
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