Limit upload batches by event count#1459
Closed
bmehta001 wants to merge 8 commits into
Closed
Conversation
Enforce the OneCollector per-request event-count limit by applying maxEventsPerUpload when upload contexts are created. Expose the new configuration key through public C++, Android, and Objective-C surfaces, and cover the default and override behavior in TPM tests. Files changed: - lib/include/public/ILogConfiguration.hpp - lib/config/RuntimeConfig_Default.hpp - lib/tpm/TransmissionPolicyManager.cpp - tests/unittests/TransmissionPolicyManagerTests.cpp - lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogConfigurationKey.java - lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java - wrappers/obj-c/ODWLogConfiguration.h - wrappers/obj-c/ODWLogConfiguration.mm Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add TPM coverage that routes an upload through StorageObserver, MemoryStorage, and Packager with maxEventsPerUpload set to 3. This proves that the configured event-count limit reaches package creation and leaves excess records queued for later batches. Files changed: - tests/unittests/TransmissionPolicyManagerTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI failure: the integration-style max-events test used fake byte blobs, which passed Release but tripped BondSplicer debug assertions because the records were not valid Bond-serialized payloads. Serialize real CsProtocol::Record payloads before storing records so the test exercises the same package path without debug/release divergence. Verified: - Win32 Debug UnitTests build and TransmissionPolicyManagerTests.* - x64 Release UnitTests build and TransmissionPolicyManagerTests.* Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new TPM configuration knob to cap the number of events included in a single upload request (defaulting to 1500, with 0 meaning unlimited), and wires that limit into upload-context creation paths so storage retrieval can respect it. It also exposes the key through Android/Objective-C wrappers and expands TPM unit test coverage to validate default behavior, overrides, and batching behavior.
Changes:
- Added
CFG_INT_TPM_MAX_EVENTS_PER_UPLOAD/maxEventsPerUpload(default1500) to the runtime configuration surface. - Applied the configured max-event-count to
EventsUploadContextcreated by both scheduled uploads and immediate (max-latency) uploads. - Exposed the key in Android and Objective-C wrappers and added/updated tests to validate default + override + packaging behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| wrappers/obj-c/ODWLogConfiguration.mm | Defines the new Objective-C configuration key constant for max events per upload. |
| wrappers/obj-c/ODWLogConfiguration.h | Declares the new Objective-C configuration key constant for max events per upload. |
| tests/unittests/TransmissionPolicyManagerTests.cpp | Adds test coverage asserting default/overridden max event count and validates batching behavior via storage retrieval. |
| lib/tpm/TransmissionPolicyManager.cpp | Sets requestedMaxCount on created upload contexts based on runtime config. |
| lib/include/public/ILogConfiguration.hpp | Adds public config key constant CFG_INT_TPM_MAX_EVENTS_PER_UPLOAD with documentation. |
| lib/config/RuntimeConfig_Default.hpp | Adds default value 1500 for CFG_INT_TPM_MAX_EVENTS_PER_UPLOAD under the TPM config map. |
| lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogConfigurationKey.java | Exposes maxEventsPerUpload through the Android configuration key enum. |
| lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java | Extends Android default-config test to assert the new default value (1500). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment 3336129493: the max-events tests modified the shared runtime configuration and manually restored it, which could leak state if a fatal ASSERT returned early. Add a scoped restoration helper and use it in both config-mutating tests so the previous value is restored on every exit path. Verified in tests/unittests/TransmissionPolicyManagerTests.cpp. Validation: - Win32 Debug UnitTests build and TransmissionPolicyManagerTests.* - x64 Release UnitTests build and TransmissionPolicyManagerTests.* Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comments 3336266349/3336266435: Obj-C wrapper comments described the max-events key as a TPM map rather than a scalar key. Update both comments to describe max events per upload request and the 0=unlimited behavior. Comments 3336266468/3336266492/3336266521: redundant mock expectations implied uploadAsync should route through the mock even though these tests call uploadAsyncParent directly. Remove the no-op expectations. Validation: - Win32 Debug UnitTests build and TransmissionPolicyManagerTests.* - x64 Release UnitTests build and TransmissionPolicyManagerTests.* Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment 3336387520: the scoped test helper restored the prior config as unsigned, which could fail to restore the exact Variant value. Store and restore the previous value as Variant. Comments 3336387612/3336387635: maxEventsPerUpload converted Variant to unsigned directly in both upload paths. Add one helper that treats non-positive values as 0/unlimited and caps oversized values at unsigned max, then use it for scheduled and immediate uploads. Comment 3336387579: remove the duplicate paused(false) call in the batching test. Validation: - Win32 Debug UnitTests build and TransmissionPolicyManagerTests.* (43 tests) - x64 Release UnitTests build and TransmissionPolicyManagerTests.* (43 tests) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CFG_INT_TPM_MAX_EVENTS_PER_UPLOAD/maxEventsPerUploadwith a default of1500unsignedmaxContext
OneCollector documents a per-request limit of 1500 events. The SDK already limits single-event and upload package size through
maxBlobSize, but previously did not bound the event count when building an upload batch. A value of0preserves the existing unlimited behavior for callers that intentionally override the default.Validation
git diff --checkmsbuild Solutions\MSTelemetrySDK.sln /m:1 /nr:false /t:Tests\UnitTests /p:Configuration=Debug /p:Platform=Win32 /p:PlatformToolset=v145Solutions\out\Debug\Win32\UnitTests\UnitTests.exe --gtest_filter=TransmissionPolicyManagerTests.*(43 tests)msbuild Solutions\MSTelemetrySDK.sln /m:1 /nr:false /t:Tests\UnitTests /p:Configuration=Release /p:Platform=x64 /p:PlatformToolset=v145Solutions\out\Release\x64\UnitTests\UnitTests.exe --gtest_filter=TransmissionPolicyManagerTests.*(43 tests)