VPAAMP-506 "flight data recorder" system - log ring buffer - phase1 - #1743
Open
pstroffolino wants to merge 14 commits into
Open
VPAAMP-506 "flight data recorder" system - log ring buffer - phase1#1743pstroffolino wants to merge 14 commits into
pstroffolino wants to merge 14 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Implements Phase 1 of a “flight data recorder” (FDR) for AAMP logging: INFO+ log messages are captured into a ring buffer even when filtered from normal output, and the buffer is dumped when an ERROR is logged to provide recent context.
Changes:
- Added
AampFlightDataRecordersingleton and wired it into core (aamplogging.cpp) and middleware (PlayerLogManager.cpp)logprintf()paths. - Added config knobs (
enableFlightDataRecorder, max lines, max seconds) and initialization viaAampConfig::ConfigureLogSettings(). - Updated build integration (root + middleware + several unit test targets) to compile/link the new FDR implementation.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
AampFlightDataRecorder.h |
New public API + data structure for FDR entries and recorder singleton |
AampFlightDataRecorder.cpp |
New ring-buffer implementation + dump/flush logic |
aamplogging.cpp |
Captures INFO+ entries into FDR; dumps/flushes on ERROR |
middleware/playerLogManager/PlayerLogManager.cpp |
Mirrors FDR integration for middleware logprintf() |
AampConfig.h |
Adds new bool/int config enums for FDR |
AampConfig.cpp |
Adds config lookup entries and calls FDR initialization from log settings |
CMakeLists.txt |
Adds FDR sources to aamp library build |
middleware/playerLogManager/CMakeLists.txt |
Adds FDR compilation unit to playerlogmanager build |
test/utests/tests/ConfigTests/CMakeLists.txt |
Adds FDR source to fix unit test link after AampConfig.cpp dependency change |
test/utests/tests/AampLogManagerTests/CMakeLists.txt |
Links FDR into log manager unit tests |
test/utests/tests/AampDrmLegacy/CMakeLists.txt |
Links FDR into DRM legacy unit tests |
test/utests/tests/AampDrmSecureClient/CMakeLists.txt |
Links FDR into DRM secure client unit tests |
test/utests/tests/DrmOcdm/CMakeLists.txt |
Links FDR into OCDM unit tests |
FLIGHT_DATA_RECORDER_IMPLEMENTATION.md |
Implementation summary / behavior claims |
FDR_DEVELOPER_GUIDE.md |
Developer-facing usage/configuration guide |
FDR_BUILD_FIX.md |
Notes about unit test build/link fixes related to FDR |
Comment on lines
+133
to
+147
| size_t write_pos_idx = mHead.fetch_add(1, std::memory_order_acq_rel); | ||
| size_t write_pos = write_pos_idx % mMaxEntries; | ||
|
|
||
| mBuffer[write_pos] = entry; | ||
|
|
||
| size_t current_count = mCount.load(std::memory_order_relaxed); | ||
| if (current_count < mMaxEntries) | ||
| { | ||
| mCount.fetch_add(1, std::memory_order_relaxed); | ||
| } | ||
| else | ||
| { | ||
| size_t tail_pos = mTail.load(std::memory_order_acquire); | ||
| mTail.compare_exchange_strong(tail_pos, tail_pos + 1, std::memory_order_release); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
@copilot this should already be addressed.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…hase 1 Implemented a Flight Data Recorder system that captures recent log history in a ring buffer and dumps it when ERROR logs occur, providing valuable debugging context for production issues. Key Features: - Lock-free circular buffer using atomic operations for thread safety - Dual eviction strategy: time-based (60s) and count-based (5000 lines) - Captures INFO, WARN, MILESTONE, and ERROR logs (TRACE/DEBUG excluded) - Automatic dump on ERROR, then flush and continue capturing - Configurable via AampConfig (enable/disable, max lines, max seconds) - Integrated into both core AAMP and middleware logging - Minimal performance impact (~5MB memory, no mutex locks) New Files: - AampFlightDataRecorder.h: FDR class interface and FDRLogEntry struct - AampFlightDataRecorder.cpp: Complete FDR implementation Modified Files: - AampConfig.h: Added 3 new config enums for FDR settings - AampConfig.cpp: Added config lookup entries and FDR initialization - aamplogging.cpp: Integrated FDR dump/capture/flush into logprintf - middleware/playerLogManager/PlayerLogManager.cpp: Integrated FDR - CMakeLists.txt: Added FDR source files to main build - middleware/playerLogManager/CMakeLists.txt: Added FDR to library build - test/utests/tests/*/CMakeLists.txt: Added FDR to test builds (5 tests) Configuration: - enableFlightDataRecorder (bool, default: false) - flightDataRecorderMaxLines (int, default: 5000) - flightDataRecorderMaxSeconds (int, default: 60) Output Format: [FDR] prefix on all dumped entries with timestamp, source, level, thread ID Phase 1 Scope (Completed): ✓ Core FDR infrastructure ✓ ERROR-only trigger ✓ Both AAMP core and middleware integration ✓ Configuration via AampConfig ✓ Lock-free circular buffer ✓ Time + line count limits ✓ Flush and continue after dump Phase 2 Scope (Future): - WARN trigger support (after WARN/ERROR audit) - Viper Player Analytics integration - Event-based error reporting to JavaScript Testing: - All unit tests building and passing - Ready for integration and manual testing
Added comprehensive documentation for Flight Data Recorder: - FDR_BUILD_FIX.md: Build troubleshooting guide for CMakeLists changes - FDR_DEVELOPER_GUIDE.md: Developer usage guide and best practices - FLIGHT_DATA_RECORDER_IMPLEMENTATION.md: Technical implementation details These documents provide reference for developers working with FDR.
Only queue logs to FDR when they are NOT being printed (i.e., when logLevelIndex < current log level threshold). This prevents INFO logs during pre-tune from being both printed to console AND queued to FDR, which would cause redundant output when FDR is later triggered. Changes: - aamplogging.cpp: Added condition to check logLevelIndex < aampLoglevel - PlayerLogManager.cpp: Added condition to check logLevelIndex < mwLoglevel Result: FDR now only captures logs that weren't already visible, serving its intended purpose without duplication.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1. AampConfig.cpp: Add bounds checking for FDR config values - Prevent negative values from becoming huge positives when cast - Clamp maxLines to [1, 100000] with default 5000 - Clamp maxSeconds to [1, 3600] with default 60 2. AampFlightDataRecorder.cpp: Add <cstdio> header - Explicitly include header for printf() to avoid relying on indirect includes 3. AampFlightDataRecorder.cpp: Fix cutoff underflow in EvictOldEntries() - Guard against now < mMaxAgeUs which can happen early after startup - Prevents entire buffer from being purged unexpectedly 4. AampFlightDataRecorder.h: Make mInitialized atomic - Avoid data race when accessed from multiple threads - Use acquire/release semantics for proper synchronization 5. AampFlightDataRecorder.cpp: Fix multi-producer race in AddEntry() - Previous logic could allow mCount to exceed mMaxEntries under concurrent logging - Use atomic fetch_add + conditional fetch_sub to maintain ring-buffer invariants 6. Documentation: Update build commands to use approved L1 workflow - FDR_DEVELOPER_GUIDE.md: Replace generic cmake commands with cd test/utests && ./run.sh - FDR_BUILD_FIX.md: Same update for consistency with repo workflow
Use brace initialization {false} instead of parenthesis (false) for atomic<bool> members to ensure proper initialization across different C++ standard library implementations. This fixes test crashes on Ubuntu CI where atomic initialization was undefined.
The previous implementation had a window where multiple concurrent producers could each observe `mCount < mMaxEntries`, increment mCount past mMaxEntries, and then attempt a silent correction with fetch_sub + fetch_add on mTail. This broke ring-buffer invariants under concurrent logging. Fix: derive overflow detection from the monotonic head/tail gap rather than a separate count that is updated non-atomically with respect to the overflow check. Each producer atomically claims a unique write slot via `mHead.fetch_add`, then uses a CAS on mTail to advance it by exactly one slot if `new_head - tail > mMaxEntries`. Only the winner of the CAS advances tail, preventing double-eviction. mCount is kept as a clamped derived value. Also add a dedicated L1 test suite (AampFlightDataRecorderTests) with 14 tests covering: initialization, enabled flag toggling, single-producer fill/overflow, multi-producer concurrency (the fixed race), Flush, Dump edge cases, and EvictOldEntries. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
…tries Topic 0: Fix double-filter bug preventing FDR from capturing info-level logs. - AAMPLOG macro gate changed from aampLoglevel to eLOGLEVEL_INFO so info+ logs always reach logprintf regardless of display threshold. - Display guard added inside logprintf to suppress emission of below-threshold logs while still allowing FDR to capture them. Topic 2: Extract emitLogLine helper for recursion-safe log emission. - New emitLogLine() function emits pre-formatted strings through the configured logging dispatch (printf/ethanlog/sd_journal) without any FDR interaction. - FDR Dump() refactored to use emitLogLine instead of raw printf, routing dump output through the correct logging dispatch. Topic 3: Add func and line fields to FDRLogEntry. - FDRLogEntry struct extended with const char* func and int line. - FormatLogEntry includes [func][line] in output to match logprintf format. - emitLogLine stub added to FDR test runner for link compatibility. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
pstroffolino
force-pushed
the
feature/VPAAMP-506
branch
from
August 6, 2026 15:56
dd63093 to
89a7628
Compare
Topic 4: Milestone queuing with WARN-triggered dump. - logprintf restructured: INFO/MIL logs are queued in FDR buffer (not emitted), WARN/ERROR trigger Dump() before emission so buffered context precedes the trigger line chronologically. - On ring buffer eviction, MIL+ entries are lazily emitted via emitLogLine so milestone logs are never silently lost. - Default flightDataRecorderMaxSeconds reduced from 60s to 15s. - Added emitLogLine stub to FakeAampLogManager and aampMocks for L1 test link compatibility. - Updated 5 AampLogManager tests to set log level before testing below-threshold emission (TRACE/INFO format tests). Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Topic 5: FDR makes tune-time log level toggling unnecessary. - Remove setLogLevel(INFO) at tune start and setLogLevel(WARN) at tune complete — INFO logs are now always captured by FDR and dumped on WARN/ERROR regardless of display threshold. - Remove LLD buffer starvation log level toggle — same reasoning; FDR captures INFO during low-buffer episodes without needing to lower the display threshold. - Remove setLogLevel(ERROR) for fake tunes — FDR captures all levels; fake tune noise stays in the ring buffer harmlessly. - Remove mIsLoggingNeeded member (no longer referenced). Note: explicit user config (trace/debug/info flags via ConfigureLogSettings + lockLogLevel) is preserved — when a user explicitly sets a log level it still takes effect. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
MIL logs were incorrectly triggering an FDR dump because the condition used >= eLOGLEVEL_WARN which includes MIL (enum value 4 > WARN 3). MIL should be queued in the FDR buffer like INFO and emitted on eviction or dump, not treated as a warning. - FDR capture: changed from (>= INFO && < WARN) to explicit (== INFO || == MIL) so MIL is correctly queued. - FDR dump trigger: changed from (>= WARN) to explicit (== WARN || == ERROR) so only warnings and errors dump the buffer. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@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.
Support for collecting recent info level log output even if it's being default filtered, so that we can reach back and belatedly log it along with a later error. This will help ensure that collected logs from devices with default log levels are more likely to be actionable, without having to change log levels and hope for reproduction.