Add workflow spans for tagging the logs with step name - #6088
Add workflow spans for tagging the logs with step name#6088Caio-Nogueira wants to merge 1 commit into
Conversation
c3251e2 to
c9820b7
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
9184d97 to
5dad480
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6088 +/- ##
=======================================
Coverage 66.60% 66.61%
=======================================
Files 402 402
Lines 115908 115908
Branches 19427 19427
=======================================
+ Hits 77205 77209 +4
+ Misses 27104 27101 -3
+ Partials 11599 11598 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5dad480 to
d8fdc7b
Compare
|
@Caio-Nogueira Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
d8fdc7b to
4b6707a
Compare
4b6707a to
3f07823
Compare
There was a problem hiding this comment.
Follow-up review. Most prior feedback was addressed — thanks for the updates.
Remaining findings:
Formatting: The file needsjust format— missing indentation in KJ_IF_SOME body, flush-left code inrunStep, misleading closing-brace comments- Misleading comment:
setWorkflowInfodocstring claims buffered trace update that doesn't happen Dead code:TraceContext::addLogadded but never calledMissing bounds check:argv[0]accessed without size check
(These comments were generated with AI assistance and may contain errors.)
3f07823 to
9dae8a7
Compare
|
The generated output of |
c76f6b2 to
0317fc5
Compare
0317fc5 to
fefe034
Compare
fhanau
left a comment
There was a problem hiding this comment.
Reviewed the tracing-related changes and left some feedback. This excludes the handling of the async storage, which I'm less familiar with.
d11edea to
bdb076d
Compare
There was a problem hiding this comment.
This PR combines too much in one.
It also seems to have feature crept since the last time I looked at it.
The changes to add workflowName and workflowInstanceId should be pulled out, especially since I'm not sure this is the right implementation but we can discuss that in the split PR.
ead1ed0 to
f8a2e26
Compare
I removed the invocation level fields from the trace event. Although I think we should think about them (could be in a separate PR). Keeping only the log-to-span attribution logic for now. |
5709bda to
51e6f5a
Compare
|
Update: Since #6608 got merged we can just rely on the internal tracing module to wrap the user's callback. I rebased with main and removed the log instrumentation logic - that comes later (EW-10672). All we need from the workflows end is to trace the |
43579bf to
49bf362
Compare
49bf362 to
e7de698
Compare
This is a proposal to add arbitrary step-related tags on console logs emitted from within workflow instances (more specifically, steps).
#6608 got merged, meaning that the tracing api is now using async context to store active spans. This means that user spans can in theory be attributed to the correct span, once the log instrumentation is changed to do so