refactor(logs): Add generic internal item batcher #7017
Conversation
philprime
requested review from
itaybre,
noahsmartin and
philipphofmann
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7017 +/- ##
=============================================
+ Coverage 85.027% 85.057% +0.030%
=============================================
Files 454 457 +3
Lines 27737 27793 +56
Branches 12159 12174 +15
=============================================
+ Hits 23584 23640 +56
+ Misses 4107 4106 -1
- Partials 46 47 +1
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2e79f5f | 1220.53 ms | 1249.35 ms | 28.82 ms |
| 6502818 | 1229.08 ms | 1245.46 ms | 16.37 ms |
| 1a4afd3 | 1224.39 ms | 1246.16 ms | 21.77 ms |
| 82f60cf | 1218.65 ms | 1238.52 ms | 19.87 ms |
| 4b6052d | 1221.04 ms | 1241.74 ms | 20.70 ms |
| f4f94f5 | 1216.24 ms | 1247.94 ms | 31.70 ms |
| 94a6b1a | 1213.39 ms | 1231.55 ms | 18.17 ms |
| e58d7bf | 1219.98 ms | 1242.39 ms | 22.41 ms |
| 8fd192f | 1202.10 ms | 1220.19 ms | 18.09 ms |
| 56f9cc6 | 1209.47 ms | 1236.84 ms | 27.37 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2e79f5f | 23.75 KiB | 928.87 KiB | 905.12 KiB |
| 6502818 | 23.75 KiB | 959.45 KiB | 935.70 KiB |
| 1a4afd3 | 23.75 KiB | 997.18 KiB | 973.43 KiB |
| 82f60cf | 23.75 KiB | 913.63 KiB | 889.88 KiB |
| 4b6052d | 24.14 KiB | 1.02 MiB | 1019.77 KiB |
| f4f94f5 | 23.75 KiB | 988.02 KiB | 964.27 KiB |
| 94a6b1a | 23.75 KiB | 902.48 KiB | 878.74 KiB |
| e58d7bf | 24.15 KiB | 1.01 MiB | 1014.91 KiB |
| 8fd192f | 23.74 KiB | 872.75 KiB | 849.01 KiB |
| 56f9cc6 | 23.75 KiB | 913.63 KiB | 889.88 KiB |
Previous results on branch: philprime/log-batcher-abstraction
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c4a716d | 1206.94 ms | 1252.17 ms | 45.23 ms |
| ca6b1c2 | 1210.87 ms | 1238.04 ms | 27.17 ms |
| 52fdb30 | 1221.07 ms | 1253.44 ms | 32.37 ms |
| a15ffdb | 1227.70 ms | 1259.98 ms | 32.28 ms |
| 30f25b8 | 1188.75 ms | 1239.15 ms | 50.40 ms |
| 27163ba | 1228.75 ms | 1257.19 ms | 28.44 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c4a716d | 24.14 KiB | 1.02 MiB | 1021.64 KiB |
| ca6b1c2 | 24.14 KiB | 1.03 MiB | 1.00 MiB |
| 52fdb30 | 24.14 KiB | 1.02 MiB | 1021.64 KiB |
| a15ffdb | 24.14 KiB | 1.03 MiB | 1.00 MiB |
| 30f25b8 | 24.14 KiB | 1.03 MiB | 1.00 MiB |
| 27163ba | 24.14 KiB | 1.03 MiB | 1.00 MiB |
- Introduced new test file `SentryItemBatcherTests.swift` to validate the behavior of the `SentryItemBatcher`. - Added tests for various scenarios including item addition, buffer size limits, timeout handling, and attribute enrichment. - Updated project configuration to include the new test file in the build settings.
|
Moving back to draft to explore protocol-based implementation. Also I noticed that my implementation changed the requirements of |
|
Further resources to consider: |
|
@sentry review |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR refactors the SentryLogBatcher into a generic batching infrastructure to prepare for future features (#6960). The refactoring extracts SentryLog.Attribute to a new public type SentryAttribute, introduces generic batching components (Batcher, BatchStorage, BatcherScope, etc.), and updates SentryLogBatcher to use the new generic implementation.
Key changes:
- Introduces generic batching infrastructure with protocol-oriented design
- Moves
SentryLog.Attributeto standaloneSentryAttributeclass with backward compatibility - Adds comprehensive test coverage for the new generic components
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
Sources/Swift/Protocol/SentryAttribute.swift |
New standalone SentryAttribute class extracted from SentryLog.Attribute |
Sources/Swift/Protocol/SentryLog.swift |
Adds type alias for backward compatibility |
Sources/Sentry/Public/SentryDefines.h |
Adds @compatibility_alias for Objective-C compatibility |
Sources/Swift/Tools/Batcher/Batcher.swift |
Core generic batcher implementation with batch storage and timer management |
Sources/Swift/Tools/Batcher/BatcherScope.swift |
Protocol and extension for applying scope attributes to batched items |
Sources/Swift/Tools/Batcher/BatcherItem.swift |
Protocol defining requirements for items that can be batched |
Sources/Swift/Tools/Batcher/BatcherConfig.swift |
Protocol for batcher configuration |
Sources/Swift/Tools/Batcher/BatchStorage.swift |
Protocol for batch storage implementations |
Sources/Swift/Tools/Batcher/InMemoryBatchStorage.swift |
In-memory implementation of batch storage |
Sources/Swift/Tools/SentryLogBatcher.swift |
Refactored to delegate to generic Batcher |
Sources/Swift/Helper/SentryDispatchQueueWrapper.swift |
Adds protocol for dispatch queue wrapper |
Sources/Swift/Core/Helper/SentryCurrentDateProvider.swift |
Adds getAbsoluteTime() method |
Tests/SentryTests/Batcher/*.swift |
Comprehensive test coverage for new generic components |
Tests/SentryTests/SentryLogBatcherTests.swift |
Updated tests with improved structure and date provider |
sdk_api.json |
API documentation changes reflecting the type rename |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
- Updated `TestCurrentDateProvider` to include `_absoluteTime` tracking during date advancements. - Modified `Batcher` to clarify synchronous behavior in documentation and ensure correct item count is passed to the callback. - Added tests to verify scope attribute application and item count during capture in `BatcherScopeTests` and `BatcherTests`.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
denrase
left a comment
denrase
left a comment
There was a problem hiding this comment.
Looks great, makes it easier to test all the individual responsibilities that were inside the batcher. Just some general questions and feedback regarding naming. 🙇♂️
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
Thanks a lot, I think we should have a quick call about this to ensure if the still WIP concept for the TelemetryProcessor / Buffer would work for us, and then align if we still want to do some changes in this PR or follow up.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
A few more minor comments.
Overview
This PR refactors the
SentryLogBatcherimplementation by extracting its core batching logic into a generic, reusableBatchercomponent using Protocol-Oriented Programming (POP). This abstraction enables future reuse for other item types (e.g., feedback items) while maintaining type safety through Swift generics.Key Changes
Protocol-Oriented Architecture
The refactoring uses Protocol-Oriented Programming with the following protocol hierarchy:
BatcherProtocol: Defines the public interface for batching operationsBatcherItem: Protocol for items that can be batched (must beEncodable)BatcherScope: Protocol for scope objects that provide context (attributes, trace IDs, user info)BatcherConfig: Protocol for batcher configuration (timeouts, limits, callbacks)BatchStorage: Protocol for storage backends (in-memory, file-based, etc.)The generic
Batcher<Storage, Item, Scope>class implementsBatcherProtocoland works with any types conforming to these protocols, enabling code reuse without sacrificing type safety.Implementation Details
Generic Batcher Implementation (
Sources/Swift/Tools/Batcher/Batcher.swift)capture()methodScope Attribute Enrichment (
Sources/Swift/Tools/Batcher/BatcherScope.swift)Storage Abstraction (
Sources/Swift/Tools/Batcher/InMemoryBatchStorage.swift)Type Migration
SentryLog.AttributetoSentryAttributeas a public typeSentryLogAttributeto maintain Hybrid SDK compatibilityBug Fixes
TestCurrentDateProvider.getAbsoluteTime()to properly update_absoluteTimein advance methodsbatchStorage.countvsbatchStorage.size)Testing
Batcher,BatcherScope, andInMemoryBatchStorageSentryLogBatchertests continue to passBackwards Compatibility
SentryLogBatchermaintains the same interfaceRelated Issues
Closes #7013
#skip-changelog