[FFL-1720] Evaluation Logging: Aggregation Engine & Test Utilities#3145
Conversation
e1704c3 to
95f4c0a
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
95f4c0a to
a0e7df9
Compare
This comment has been minimized.
This comment has been minimized.
| // Take atomic snapshot of statistics to prevent torn reads | ||
| val snapshotCount: Int | ||
| val snapshotFirst: Long | ||
| val snapshotLast: Long | ||
| val snapshotMessage: String? | ||
| synchronized(this) { | ||
| snapshotCount = count | ||
| snapshotFirst = firstEvaluation | ||
| snapshotLast = lastEvaluation | ||
| snapshotMessage = lastErrorMessage | ||
| } |
There was a problem hiding this comment.
minor: this piece is confusing. These fields are volatile, so torn read is not possible. Otherwise, this class is not designed to handle concurrent calls to recordEvaluation and toEvaluationEvent as that may leave some events uncounted.
There was a problem hiding this comment.
perhaps this is being overly defensive. Because of the CAS of the map of aggregation stats for flushing, _no other thread should be calling toEvaluationEvent or recordEvaluation on this AggregationStats instance.
Without this synchronized, recordEvaluation could change the computed values while we're getting them for constructing the FlagEvaluation
There was a problem hiding this comment.
With the synchronized CAS snapshotting the aggregationMap for flushing, this method should be protected from another thread calling recordEvaluation which could change the values as they're being grabbed for the FlagEvaluation constructor
Is this too defensive?
There was a problem hiding this comment.
I drafted a small proposal to reduce the number of concurrency primitives usage 33544d7#r177031080 by making a trade-off for the immutability (which is more Kotlin-idiomatic). Feel free to adopt it or to reject it, whatever works for you. Performance penalty is negligible, but in exchange we reduce the number of code sites where we can run into concurrency bugs (which are hard to debug).
There we leverage event metadata to do the lightweight deserialization before the upload to drop events we are not interested in.
As of aggregation logic itself: we have a bit similar place in the SDK, where we reduce views, but there is it is in the boundaries of a single batch file and also the same model is used as a result (while here models are different).
There was a problem hiding this comment.
@dd-oleksii Cannot answer your comments on the commit directly, because new Github UI doesn't show them to me.
🐛 it's possible that timestamp is less than lastEvaluation, in which case lastEvaluation should not be updated. Similarly for firstEvaluation — it's possible that timestamp < firstEvlauation
Yes, it is a draft commit, I didn't 100% port this logic, but it is easy to fix with min / max functions.
🐛 Splitting size check and drain into different lock sections introduces a race condition and allows draining multiple times when size limit is exceeded (the number of drains is not limited to two).
Yes, a small sacrifice to have lighter locking. Worst case we just drain preliminary. Otherwise can be addressed by doing the size check again in the drain function inside the lock.
There was a problem hiding this comment.
Thanks @0xnm .
I have applied the proposed change, fixed the timestamp bug and shifted the locking to allow a second check of the map size within the lock in order to limit to 1 drain. PTAL
eb13adb to
c924067
Compare
97e4a4e to
eb0ace5
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/flags-evaluations-logging #3145 +/- ##
=====================================================================
+ Coverage 70.81% 70.97% +0.16%
=====================================================================
Files 901 905 +4
Lines 33172 33310 +138
Branches 5596 5614 +18
=====================================================================
+ Hits 23489 23641 +152
+ Misses 8118 8102 -16
- Partials 1565 1567 +2
🚀 New features to boost your workflow:
|
c924067 to
a031b97
Compare
968dc68 to
80c8de7
Compare
ddeaaa3 to
75827d0
Compare
2eb16ca to
0cfb30a
Compare
0cfb30a to
141cded
Compare
1c0c827 to
00b8f55
Compare
141cded to
4bfc37e
Compare
00b8f55 to
7dba2fe
Compare
4bfc37e to
550f9f0
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
| aggregationMap.size | ||
| } | ||
|
|
||
| if (mapSize < maxAggregations) { | ||
| return emptyList() | ||
| } | ||
|
|
||
| // Re-check inside lock to ensure only one thread drains | ||
| val drained = mapLock.withLock { |
There was a problem hiding this comment.
minor: given this is the same lock we re-acquire, we don't need to release it and can simply proceed to draining.
| aggregationMap.size | |
| } | |
| if (mapSize < maxAggregations) { | |
| return emptyList() | |
| } | |
| // Re-check inside lock to ensure only one thread drains | |
| val drained = mapLock.withLock { |
| ) | ||
|
|
||
| if (drainedEvents.isNotEmpty()) { | ||
| writer.writeAll(drainedEvents) |
dd-oleksii
left a comment
dd-oleksii
left a comment
There was a problem hiding this comment.
Looks good overall. Added two minor comments above — let's address them in a follow up PR given how long this one is hanging
adb81d5
into
feature/flags-evaluations-logging
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
4 participants
🥞 Evaluation Logging Stacked Pull Requests 🥞
🔲 Integration & Configuration (PR #3147)
🔲 Storage & Network Infrastructure (PR #3146)
👉 Aggregation Engine & Test Utilities (this PR)
☑️ FlagEvaluation Schema (PR #3166)
☑️ Event Schema & Data Models (PR #3144)
☑️ Evaluations Subfeature (PR #3159)
⎿
feature/flags-evaluations-logging(feature branch)Datadog Internal
🎟️ Ticket: FFL-1720 - Implement Evaluation Logging for Android SDK
What does this PR do?
Implements the core evaluations aggregation engine that groups feature flag evaluations by key characteristics and manages periodic flushing from memory to persistent storage (via
StorageBackedFeature/EvaluationsFeature).The
EvaluationEventProcessormanages concurrent access and mutation ofAggregationStatsinstances in memory. Each Stats object is given anAggregationKey- a composite key made of the set of identifiers across the evaluation (targetingKey, variationKey, etc.) and SDK context (rum View ID). This ensures proper evaluation counting under the "same" circumstances.The processor also manages the periodic and size-based triggers for flushing the in-memory aggregations to persistent storage.
Motivation
We need to implement Evaluation Logging to provide comprehensive visibility into all feature flag evaluations, including defaults, errors, and successful matches. This goes beyond exposure logging by capturing aggregated metrics about evaluation frequency, error rates, and runtime default usage across all flags.
Description
This PR adds the core business logic for evaluation logging:
Implementation:
AggregationKey: Defines how evaluations are grouped (by flag, variant, allocation, targeting key, and error code)AggregationStats: Tracks count, first/last timestamps, and last error message for each aggregationEvaluationEventsProcessor: Orchestrates aggregation, manages flush triggers (time-based, size-based, shutdown)EvaluationEventWriter: Interface for persisting evaluation events (implementation in PR [FFL-1720] Evaluation Logging: Storage & Network Infrastructure #3146)Additional Notes
Review checklist (to be filled by reviewers)