Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: process metrics later in executeTemplate. Fixes #13162 #13163

Merged
merged 12 commits into from
Jun 18, 2024
Merged

fix: process metrics later in executeTemplate. Fixes #13162 #13163

merged 12 commits into from
Jun 18, 2024

Conversation

dpadhiar
Copy link
Member

Fixes #13162

Motivation

Failure metric when defined in WorkflowTemplate wasn't being reported.

Modifications

Changed where the processedMetrics are checked during reconciliation such that the current Node shows as Failed and the metric is processed correctly.

Verification

Ran example workflow from initial issue to test that metric is now showing.

@dpadhiar
fix: move where metrics are processed in execTemplate
2616e34 
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
@dpadhiar dpadhiar requested review from juliev0 and sarabala1979 June 10, 2024 21:49
agilgur5
agilgur5 suggested changes Jun 10, 2024
View reviewed changes
Copy link

@agilgur5 agilgur5 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran example workflow from initial issue to test that metric is now showing.

Please do add a regression test for this. Otherwise, I'm not an expert in the templating part of the codebase so I'll let others review

@agilgur5 agilgur5 added area/metrics area/templating Templating with `{{...}}` labels Jun 10, 2024
@dpadhiar
Copy link
Member Author

dpadhiar commented Jun 10, 2024 •
edited by agilgur5
Loading

Will do, can add a test case for using the metrics in a template.

dpadhiar added 4 commits June 10, 2024 16:16
@dpadhiar
test: metric test for failed metrics
74d59c3 
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
@dpadhiar
fix: typo
dec8d21 
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
@dpadhiar
chore: formatting
021597a 
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
@dpadhiar
chore: trigger test
6099044 
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
agilgur5
agilgur5 reviewed Jun 11, 2024
View reviewed changes
Copy link

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but it looks to me like the test case can be quite heavily simplified. We should keep tests as concise and isolated as possible, using only the features we are trying to test

There are also leftover test comments in it that shouldn't make it into the codebase.
And some style issues

test/e2e/testdata/template-failed-metric.yaml Outdated Show resolved Hide resolved
test/e2e/testdata/template-failed-metric.yaml Outdated Show resolved Hide resolved
test/e2e/metrics_test.go Outdated Show resolved Hide resolved
test/e2e/testdata/template-failed-metric.yaml Outdated Show resolved Hide resolved
test/e2e/testdata/template-failed-metric.yaml Outdated Show resolved Hide resolved
test/e2e/testdata/template-failed-metric.yaml Outdated Show resolved Hide resolved
test/e2e/testdata/template-failed-metric.yaml Outdated Show resolved Hide resolved
test/e2e/testdata/template-failed-metric.yaml Outdated Show resolved Hide resolved
@dpadhiar
Copy link
Member Author

Correct me if I'm wrong, but it looks to me like the test case can be quite heavily simplified. We should keep tests as concise and isolated as possible, using only the features we are trying to test

There are also leftover test comments in it that shouldn't make it into the codebase. And some style issues

You are right, I did rush a bit to push the PR in (customer issue at work) so let me go ahead and clean it up / address your comments today.

dpadhiar added 2 commits June 11, 2024 09:46
@dpadhiar
test: renaming and simplifying
4d05e77 
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
@dpadhiar
Merge branch 'master' into temp-metric
f96b43b 
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
@dpadhiar dpadhiar requested a review from agilgur5 June 11, 2024 18:08
@dpadhiar dpadhiar marked this pull request as ready for review June 14, 2024 20:14
juliev0
juliev0 reviewed Jun 17, 2024
View reviewed changes
workflow/controller/operator.go
woc.computeMetrics(processedTmpl.Metrics.Prometheus, localScope, realTimeScope, false)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify how moving the code helped? What is it about the lines above this code that they needed to be executed first?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, from what I went over with @sarabala1979, this line retrieveNode, err := woc.wf.GetNodeByName(node.Name) gives us the most up to date node for the workflow. This one will have the correct Fulfilled status and this will let us go through this logic prevNodeStatus, ok := woc.preExecutionNodePhases[node.ID]; (!ok || !prevNodeStatus.Fulfilled()) && node.Fulfilled() { successfully to be able to call computeMetrics for a failed Workflow.

dpadhiar added 2 commits June 17, 2024 09:13
@dpadhiar
test: move test to only Workflow
164a289 
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
@dpadhiar
Merge branch 'temp-metric' of https://github.com/dpadhiar/argo-workflows
750dfb6 
 into temp-metric

Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
@agilgur5 agilgur5 changed the title fix: move where metrics are processed in executeTemplate. Fixes #13162 fix: process metrics later in executeTemplate. Fixes #13162 Jun 17, 2024
@dpadhiar
test: renaming
010b9e3 
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
agilgur5
agilgur5 approved these changes Jun 17, 2024
View reviewed changes
Copy link

@agilgur5 agilgur5 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests LGTM now. Will let others review the rest

sarabala1979
sarabala1979 approved these changes Jun 18, 2024
View reviewed changes
Copy link
Member

@sarabala1979 sarabala1979 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks @dpadhiar. It is a good catch.
Thanks @agilgur5 and @juliev0 to review that.

@sarabala1979 sarabala1979 merged commit 6201d75 into argoproj:main Jun 18, 2024
28 checks passed
agilgur5 pushed a commit that referenced this pull request Jul 6, 2024
@dpadhiar
fix: process metrics later in executeTemplate. Fixes #13162 (#13163)
5c85fd3 
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
(cherry picked from commit 6201d75)
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliev0 juliev0 juliev0 left review comments

@agilgur5 agilgur5 agilgur5 approved these changes

@sarabala1979 sarabala1979 sarabala1979 approved these changes

Assignees
No one assigned
Labels
area/metrics area/templating Templating with `{{...}}`
Projects
None yet
Milestone
v3.5.x patches
Development

Successfully merging this pull request may close these issues.

Metrics Error: failure metric for workflow status not reporting correctly if in WorkflowTemplate
4 participants
@dpadhiar @agilgur5 @juliev0 @sarabala1979