-
Notifications
You must be signed in to change notification settings - Fork 124
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
RUM-1837 Update logic to send N batches sequentially in each cycle #1531
RUM-1837 Update logic to send N batches sequentially in each cycle #1531
Conversation
804a108
to
dceec59
Compare
Datadog ReportBranch report: ❌ ❌ Failed Tests (1)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will this improvement be measured? During backlog grooming we discussed adding "batch processing level" to "Batch Deleted" telemetry, so we can correlate batch delivery with certain configuration. This part of definition of done seems lacking.
DatadogCore/Tests/Datadog/Core/Persistence/Reading/FileReaderTests.swift
Outdated
Show resolved
Hide resolved
c69045b
to
53b36cc
Compare
Datadog ReportBranch report: ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I see room for improvement in perfs: our DataUploader
uses a semaphore which will be blocking the thread. So I think we should balance the load instead of uploading in a loop. I left a suggestion in comment, LMKWYT!
@maxep @ncreated Since files are symlinks turned into data just before upload synchronously - we just see extended allocation, but it's not a spike. In my tests it was way faster to upload everything when using 10-batches - which is a goal of the PR. Using multiple I think that there is a potential in refactoring this into a OperationQueue, but it will become quite a big change. I think it's better iterate on existing setup first. |
4104f67
to
daa7235
Compare
Datadog ReportBranch report: ❄️ ❌ Failed Tests (1)
New Flaky Tests (1)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @maciejburda 👏
I've left one minor comment, but it looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the updates 🙏
What and why?
Modifies the core logic to get a list of batches instead of single batch. It's done through a new configuration called
BatchProcessingLevel
that allows controlling the amount of batches processed sequentially without a delay within one reading/uploading cycle. Currently it exposed 3 levels:low
,medium
andhigh
that translate to1
,10
and100
of batches processed. By default it's taking up to10
batches in a cycle.This logic improves the data upload when batch back pressure occurs.
How?
Configuration is delivered to
DataUploadWorker
which implementation was modified to read up to X batches instead of single batch.Review checklist
Custom CI job configuration (optional)
tools/