-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: Custom metrics are not recorded for DAG tasks Fixes #3872 #3886
Conversation
workflow/controller/dag.go
Outdated
// Collect the completed task metrics | ||
tmpl := woc.wf.GetTemplateByName(task.Template) | ||
if tmpl != nil && tmpl.Metrics != nil { | ||
if prevNodeStatus, ok := woc.preExecutionNodePhases[node.ID]; ok && !prevNodeStatus.Fulfilled() { | ||
localScope, realTimeScope := woc.prepareMetricScope(node) | ||
woc.computeMetrics(tmpl.Metrics.Prometheus, localScope, realTimeScope, false) | ||
} | ||
} |
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.
Not too sure this is the correct approach here. All metrics should be emitted within executeTemplate
. If you notice for DAG or Steps, metrics are never emitted outside of it. Can you investigate why the current metric emission code in executeTemplate
is not getting called in this case?
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.
In DAG flow, the Code is checking the node.Fulfilled()
in early-stage (executeDAGTask) which prevents to execute the executeTemplate
for completed the tasks.
https://github.com/argoproj/argo/blob/5b5d2359ef9f573121fe6429e386f03dd8652ece/workflow/controller/dag.go#L316-L326
Options:
- We can remove the
return
statement on the above code. So, Completed node will travel toexecutetemplate
But I am not sure whether it will break any DAG flow - Above fix, Add metrics emission code in
node.Fulfilled()
check
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.
On second thought, I think we can keep this current fix for this issue.
workflow/controller/dag.go
Outdated
// Collect the completed task metrics | ||
tmpl := woc.wf.GetTemplateByName(task.Template) | ||
if tmpl != nil && tmpl.Metrics != nil { | ||
if prevNodeStatus, ok := woc.preExecutionNodePhases[node.ID]; ok && !prevNodeStatus.Fulfilled() { | ||
localScope, realTimeScope := woc.prepareMetricScope(node) | ||
woc.computeMetrics(tmpl.Metrics.Prometheus, localScope, realTimeScope, false) | ||
} | ||
} |
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.
On second thought, I think we can keep this current fix for this issue.
Thank you for taking care of the issue that fast! Highly appreciated |
Backported to 2.10 and 2.11 |
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
. Fixes Custom metrics are not recorded for DAG tasks. #3872