feat: add timestamp parsing for container execution logs (#1343)#1740
Open
sufirehman wants to merge 1 commit into
Open
feat: add timestamp parsing for container execution logs (#1343)#1740sufirehman wants to merge 1 commit into
sufirehman wants to merge 1 commit into
Conversation
…reen-coding-solutions#1343) Pass `--timestamps` to `docker logs` so each line is prefixed with an RFC3339Nano timestamp. A new module-level helper `_parse_timestamped_log_lines()` parses those prefixed lines into `{timestamp, content}` dicts, falling back to `{timestamp: None, ...}` for lines that don't match. `_add_to_current_run_log()` now branches on log type: CONTAINER_EXECUTION entries store `stdout_entries`/`stderr_entries` (lists of the parsed dicts) instead of plain strings. All other log types (SETUP_COMMAND, FLOW_COMMAND, NETWORK_STATS, EXCEPTION) are unchanged and continue to use plain `stdout`/`stderr` keys. Tests cover the parser (normal, fallback, empty input) and confirm the correct key shape for CONTAINER_EXECUTION, SETUP_COMMAND, and FLOW_COMMAND log entries.
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
1 participant
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.
Description
Implements Option 1 (Integrated Timestamp Storage) from the issue, scoped specifically to LogType.CONTAINER_EXECUTION. Adds the --timestamps flag to the docker logs call in _read_container_logs(), parses each line's RFC3339Nano timestamp prefix into {timestamp, content} entries, and stores them as stdout_entries/stderr_entries for container logs only. SETUP_COMMAND and FLOW_COMMAND log types keep their existing plain stdout/stderr string structure completely unchanged.
Related Issue
Closes #1343
Motivation and Context
Container logs span multiple phases (BOOT, IDLE, RUNTIME) and are currently stored as a single undifferentiated block per phase. Docker's --timestamps flag gives per-line RFC3339Nano timestamps for free, which lets us time-key container log lines for future phase-splitting and analysis, as proposed by @ArneTR.
How Has This Been Tested?
Added 6 tests: timestamp parsing on normal docker --timestamps output, fallback to {timestamp: None, content: line} for any line that doesn't match the expected format, empty input handling, and three tests confirming CONTAINER_EXECUTION entries use the new structure while SETUP_COMMAND and FLOW_COMMAND entries remain plain strings, completely unaffected. All 6 tests pass. (Note: the existing test teardown calls Tests.reset_db(), which requires the test Postgres container to be running; this is unrelated to the change and fails identically on main without the test Docker stack started.)
Scope notes
Per discussion with @ArneTR, this PR does not touch frontend display. Per his guidance, the plan for a follow-up is to simply prepend each line with its timestamp in the Logs tab, similar to how
docker logs -tdisplays output, with no interleaving logic for now.Types of changes