RUM-16305: Update several hardcoded data limits#3616
RUM-16305: Update several hardcoded data limits#3616aleksandr-gringauz wants to merge 2 commits into
Conversation
aleksandr-gringauz
force-pushed
the
aleksandr-gringauz/RUM-16305/fix-limits
branch
3 times, most recently
from
4481575 to
252d193
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3616 +/- ##
===========================================
+ Coverage 73.27% 73.29% +0.01%
===========================================
Files 995 995
Lines 36258 36258
Branches 6125 6125
===========================================
+ Hits 26568 26572 +4
- Misses 7979 7986 +7
+ Partials 1711 1700 -11
🚀 New features to boost your workflow:
|
aleksandr-gringauz
force-pushed
the
aleksandr-gringauz/RUM-16305/fix-limits
branch
from
252d193 to
214860f
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 214860fb54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| maxItemSize = 1024L * 1024, | ||
| maxItemsPerBatch = 1000, | ||
| // 5 MB | ||
| maxBatchSize = 5L * 1024 * 1024, |
There was a problem hiding this comment.
With this default set to the full 5 MB intake limit, a batch can grow past that limit before upload: SdkFeature copies this value into FilePersistenceConfig, and BatchFileOrchestrator only checks lastFile.lengthSafe(...) < config.maxBatchSize before FileEventBatchWriter appends another event that is now allowed to be up to 1 MB. When a file is just under 5 MB, the next valid event can therefore produce a request over 5 MB (plus request separators/prefixes), and the uploader treats a 413 as a non-retryable client error, so that batch is dropped. Please keep headroom here or change the write path to check current size plus the next event before appending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a valid point actually...
| maxBatchSize = 4L * 1024 * 1024, | ||
| // 1 MB | ||
| maxItemSize = 1024L * 1024, | ||
| maxItemsPerBatch = 1000, |
There was a problem hiding this comment.
This default is also inherited by FeatureStorageConfiguration.DEFAULT.copy(...) callers that don't override maxItemsPerBatch. The Session Replay, SR resources, and WebView replay storage configs still document a 500-item batch cap while only overriding item and byte sizes, so small replay/resource payloads can now be grouped into 1000-item uploads instead of the intended 500. Please override maxItemsPerBatch = 500 in those custom configs (or update them deliberately) rather than changing the global default out from under them.
Useful? React with 👍 / 👎.
| internal const val MAX_ITEM_SIZE: Long = 1024L * 1024 // 1 MB | ||
| internal const val OLD_FILE_THRESHOLD: Long = 18L * 60L * 60L * 1000L // 18 hours | ||
| internal const val MAX_DISK_SPACE: Long = 128 * MAX_BATCH_SIZE // 512 MB | ||
| internal const val MAX_DISK_SPACE: Long = 128 * MAX_BATCH_SIZE // 640 MB |
There was a problem hiding this comment.
Because BatchFileOrchestrator.freeSpaceIfNeeded() purges only after config.maxDiskSpace, deriving this from the new 5 MB batch size raises each storage directory's cap from the documented 512 MB to 640 MB. On apps with multiple storage-backed features and both pending/granted directories, this lets the SDK retain substantially more data on device even though the PR is only aligning intake payload limits; please keep the disk cap fixed unless the storage budget is intentionally being changed and documented.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8523c51219
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| val isRecentEnough = isFileRecent(lastFile, recentWriteDelayMs) | ||
| val hasRoomForMore = lastFile.lengthSafe(internalLogger) < config.maxBatchSize | ||
| val hasRoomForMore = lastFile.lengthSafe(internalLogger) < config.maxBatchSize - config.maxItemSize |
There was a problem hiding this comment.
For logs/RUM/traces batches that are just under maxBatchSize - maxItemSize, this still reuses the file and accepts another event up to maxItemSize, but the uploaded body is not only the raw event bytes: LogsRequestFactory adds [/,/] framing and RUM/traces join events with separators. That leaves zero bytes for request framing, so a max-size final event can still produce a 413, which DataUploadRunnable drops as a non-retryable client error; please keep additional overhead headroom or check the actual serialized body size before reusing the batch.
Useful? React with 👍 / 👎.
|
|
||
| val isRecentEnough = isFileRecent(lastFile, recentWriteDelayMs) | ||
| val hasRoomForMore = lastFile.lengthSafe(internalLogger) < config.maxBatchSize | ||
| val hasRoomForMore = lastFile.lengthSafe(internalLogger) < config.maxBatchSize - config.maxItemSize |
There was a problem hiding this comment.
For Session Replay, SR resources, and WebView replay, the storage configs set maxItemSize equal to maxBatchSize (10 MB), so this new threshold becomes lastFile.lengthSafe(...) < 0 for every existing batch. After the first small replay record/resource is written, the next write can never reuse that batch, effectively turning the documented 1000-item cap into one upload per item and losing SR segment/resource batching; please base the decision on the actual next event size or special-case equal item/batch limits.
Useful? React with 👍 / 👎.
3 participants
What does this PR do?
Aligns several hardcoded client-side data limits with the documented backend limits. Mirrors the equivalent iOS change in dd-sdk-ios#2832.
Changes
4 MB → 5 MB.500 → 1000.512 KB → 1 MB.Motivation