-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(logs): always capture cost, logging size failures #2487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR fixes a critical bug where execution costs were not captured when logs failed to write to the database due to size constraints. The implementation adds three key improvements:
Key Issue Found:
Recommendation:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant WE as Workflow Executor
participant LS as LoggingSession
participant EL as ExecutionLogger
participant DF as DisplayFilters
participant DB as Database
WE->>LS: safeComplete(traceSpans)
LS->>LS: complete(traceSpans)
LS->>EL: completeWorkflowExecution(traceSpans)
EL->>DF: filterForDisplay(traceSpans)
Note over DF: Truncate strings to 100K<br/>Remove null bytes<br/>Handle circular refs
DF-->>EL: filtered traceSpans
EL->>DB: Store execution log
alt Database write fails (too large)
DB-->>EL: Error
EL-->>LS: Error
LS->>LS: completeWithCostOnlyLog()
LS->>LS: calculateCostSummary(traceSpans)
Note over LS: Extract costs from<br/>oversized traceSpans
LS->>EL: completeWorkflowExecution(empty traceSpans)
Note over LS: Pass empty array to avoid<br/>size issues
EL->>DF: filterForDisplay([])
DF-->>EL: []
EL->>DB: Store cost-only log
DB-->>EL: Success
Note over DB: Cost captured,<br/>traceSpans empty
end
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 4 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 2 comments
Summary
Type of Change
Testing
Testing with @aadamgough
Checklist