Repository navigation
refactor: simplify workflow log output - #2512
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds run-writer filtering, standardizes structured DAG and sub-DAG log fields, propagates execution context through agent logging, and adds Activity and Raw modes for execution-log display with scheduler-log parsing. ChangesExecution logging and display
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExecutionLog
participant parseSchedulerLogLine
participant ActivityLine
ExecutionLog->>parseSchedulerLogLine: Parse each scheduler log line
parseSchedulerLogLine-->>ExecutionLog: Return SchedulerLogLine
ExecutionLog->>ActivityLine: Render parsed activity entry
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmn/logger/logger.go`:
- Around line 150-156: Update isRunContext in internal/cmn/logger/logger.go to
classify both root and parent as run-context attributes. In
internal/cmn/logger/logger_test.go, extend baseContext with root and parent,
then verify run output omits both while central output retains them.
In `@internal/runtime/agent/agent.go`:
- Around line 685-700: Add an Agent regression test covering the sub-DAG
scheduler-writer path around withSubDAGRunSchedulerLog, including logger
replacement and central-field restoration. Verify normal scheduler output
excludes ambient run fields, while the attempt ID appears only in the captured
attemptBoundaryLogger lifecycle output.
In `@internal/service/frontend/api/v1/dagruns.go`:
- Around line 1055-1056: The failure log in GetDAGRunOutputs currently uses the
requested run ID, which may be "latest", instead of the resolved attempt ID.
Update the tag.RunID argument to use the resolved DAGRunAttempt’s run ID while
preserving the existing logging context and behavior.
In `@ui/src/features/dags/components/dag-execution/ExecutionLog.tsx`:
- Around line 61-63: Update both structured and unstructured message containers
in ExecutionLog, including the div containing AnsiLine, to add the
whitespace-normal and break-words classes so long unbroken Activity messages
wrap within the log panel.
- Around line 636-640: Restore Activity mode line targeting by passing
getLineNumber(index) from the activityLines.map call to ActivityLine, then
render that value as the data-line-number attribute in ActivityLine so
handleJumpToLine can locate, scroll to, and highlight the target. Add a
regression test covering jump-to-line behavior in Activity mode.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 30136b8f-8a86-4504-bfd6-6f67d970c73d
📒 Files selected for processing (26)
internal/cmd/agent_executor.gointernal/cmd/context.gointernal/cmd/restart.gointernal/cmn/logger/logger.gointernal/cmn/logger/logger_test.gointernal/cmn/telemetry/propagation.gointernal/runtime/agent/agent.gointernal/runtime/agent/dbclient.gointernal/runtime/builtin/chat/tool_executor.gointernal/runtime/builtin/dag/enqueue.gointernal/runtime/builtin/dag/parallel.gointernal/runtime/executor/dag_runner.gointernal/runtime/manager.gointernal/runtime/runner.gointernal/service/chatbridge/monitor.gointernal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/humantasks.gointernal/service/frontend/api/v1/queues.gointernal/service/frontend/api/v1/webhooks.gointernal/service/scheduler/queue_dispatcher.gointernal/service/scheduler/zombie_detector.gointernal/subflow/runner.goui/src/features/dags/components/dag-execution/ExecutionLog.tsxui/src/features/dags/components/dag-execution/LogSideModal.tsxui/src/lib/__tests__/scheduler-log.test.tsui/src/lib/scheduler-log.ts
💤 Files with no reviewable changes (2)
- internal/runtime/runner.go
- ui/src/features/dags/components/dag-execution/LogSideModal.tsx
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/runtime/agent/agent_test.go (1)
184-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck context filtering on every step-scoped record.
The test checks only the
"Step started"record. A later record with a"step"field can contain inherited context and this test will still pass. Check the filtered keys for every record that contains"step".Proposed test update
for _, record := range records { if _, ok := record["attempt-id"]; ok { attemptCount++ } + if _, isStepRecord := record["step"]; isStepRecord { + for _, key := range []string{"dag", "run-id", "attempt-id", "worker-id", "trace-id", "span-id", "trace-flags", "root", "parent"} { + require.NotContains(t, record, key) + } + } switch record["msg"] { case "DAG run started": boundary = record case "Step started": step = record } } require.Equal(t, attemptID, boundary["attempt-id"]) require.Equal(t, 1, attemptCount) require.Equal(t, "collect_metrics", step["step"]) -for _, key := range []string{"dag", "run-id", "attempt-id", "worker-id", "trace-id", "span-id", "trace-flags", "root", "parent"} { - require.NotContains(t, step, key) -}As per coding guidelines, “Add or update tests appropriate to the changed code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/runtime/agent/agent_test.go` around lines 184 - 186, The test currently validates filtered context only for the “Step started” record. Update the assertions in the relevant agent test to iterate over every record containing a “step” field and verify none includes the filtered keys, preserving the existing key list and failure behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/runtime/agent/agent_test.go`:
- Around line 184-186: The test currently validates filtered context only for
the “Step started” record. Update the assertions in the relevant agent test to
iterate over every record containing a “step” field and verify none includes the
filtered keys, preserving the existing key list and failure behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 11350f7e-af06-413a-92b6-ada1b7c32cd8
📒 Files selected for processing (8)
internal/cmn/logger/logger.gointernal/cmn/logger/logger_test.gointernal/runtime/agent/agent_test.gointernal/runtime/node.gointernal/service/frontend/api/v1/dagruns.goui/src/features/dags/components/dag-execution/ActivityLine.tsxui/src/features/dags/components/dag-execution/ExecutionLog.tsxui/src/features/dags/components/dag-execution/__tests__/ExecutionLog.test.tsx
💤 Files with no reviewable changes (1)
- internal/runtime/node.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cmn/logger/logger_test.go
- internal/cmn/logger/logger.go
- internal/service/frontend/api/v1/dagruns.go
- ui/src/features/dags/components/dag-execution/ExecutionLog.tsx
Member
Author
|
Addressed the follow-up nitpick in |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
|
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.
Summary
Why
Workflow log lines repeated DAG, run, attempt, worker, and trace identity even when the surrounding file or page already established that context. This made the useful event, step, and error information unnecessarily difficult to scan. Some paths also used aliases or repeated correlation keys, which made centralized search and aggregation less reliable.
Impact
Run files and the Activity UI now emphasize the event-specific fields. Centralized logs retain
dag,run-id,attempt-id, worker identity, and valid trace context so records remain independently searchable. Existing raw text and JSON log formats remain supported.Validation
go test ./internal/cmn/logger ./internal/runtime/agent ./internal/runtime ./internal/runtime/builtin/chat ./internal/runtime/builtin/dag ./internal/runtime/executorgo test ./internal/cmd ./internal/service/chatbridge ./internal/service/frontend/api/v1 ./internal/service/scheduler ./internal/subflowgo test ./internal/cmn/telemetrypnpm test -- scheduler-logpnpm exec vitest run src/lib/__tests__/scheduler-log.test.tsSummary by cubic
Simplified workflow log output and added a readable Activity view that highlights event details. Step names are now easy to scan, and run-scoped files drop redundant context while centralized logs stay searchable.
New Features
ActivityLine.parseSchedulerLogLineparses text/JSON scheduler logs, hiding run context and surfacing meaningful details.Refactors
WithRunWriterto omit ambient run context (dag,run-id,attempt-id,worker-id, trace fields, parent/root) in run-scoped outputs; used for run files, restart path, and sub‑DAG scheduler logs.tag.DAG,tag.RunID,tag.SubDAG, andtag.SubRunID; include worker ID when present; attach trace context once; reduced telemetry debug noise.Written for commit e09bbc5. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes