feat(controller): add targetRevision in metric app_info #15143

ebuildy · 2023-08-21T22:09:46Z

Add "targetRevision" for argocd_app_info metric.

--> Should fix #4115

This is useful for:

production env: check apps are using master branch
dev env: check apps are not using master branch
alert on drift: "A devops guy could change manually an app revision in order to test some code changes, I want to be alerted after 2 days in order to rollback the app revision to master"

codecov · 2023-08-22T23:36:31Z

Codecov Report

Attention: Patch coverage is 94.11765% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 49.25%. Comparing base (f87897c) to head (2449b16).
Report is 102 commits behind head on master.

❗ Current head 2449b16 differs from pull request most recent head 9b7215d. Consider uploading reports for the commit 9b7215d to get more accurate results

Files	Patch %	Lines
cmd/argocd/commands/admin/app.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15143      +/-   ##
==========================================
- Coverage   49.73%   49.25%   -0.49%     
==========================================
  Files         274      274              
  Lines       48948    48119     -829     
==========================================
- Hits        24343    23699     -644     
+ Misses      22230    22076     -154     
+ Partials     2375     2344      -31

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ashishkcs

This looks good and will be nice to have this in.

ashishkcs · 2023-09-05T03:52:27Z

@alexmt @jessesuen could you please approve this?

sosoriov · 2023-10-04T12:06:05Z

What is the current status of this?

aspyker · 2023-10-15T06:47:51Z

Would also like to know if / when this will be merged.

mikejoh · 2023-11-29T21:47:33Z

We would also like to see this feature added but when reading up on this there's been a discussion regarding the risk of an explosion of metric cardinality due to the uniqueness of the label revision. One person suggested a way of making it optional to add revision as label to argocd_app_info which sounds like a good idea. How about revisiting that in this PR?

ebuildy · 2023-11-29T22:43:19Z

We would also like to see this feature added but when reading up on this there's been a discussion regarding the risk of an explosion of metric cardinality due to the uniqueness of the label revision. One person suggested a way of making it optional to add revision as label to argocd_app_info which sounds like a good idea. How about revisiting that in this PR?

I could add a revision whitelist as setting:

metricRevisions: [master, develop]

If branch is not whitelisted -> "other"

what do u think?

mikejoh · 2023-11-30T07:36:53Z

I could add a revision whitelist as setting:

metricRevisions: [master, develop]

If branch is not whitelisted -> "other"

what do u think?

I think that if we're about to add this label we should keep all possible values. Adding a feature toggle via e.g. a flag and clearly explaning what the flag does would be another approach. In the end maybe a combination of both approaches?

ebuildy · 2023-11-30T14:20:54Z

I could add a revision whitelist as setting:
metricRevisions: [master, develop]
If branch is not whitelisted -> "other"
what do u think?

I think that if we're about to add this label we should keep all possible values. Adding a feature toggle via e.g. a flag and clearly explaning what the flag does would be another approach. In the end maybe a combination of both approaches?

Well, the whitelist is a good idea and we could switch off this feature if whitelist is empty.

Possible whitelist values:

"" --> switch off
"master" --> only master branch, other revisions are labelized as "other"
"master,develop"
"*" --> switch on, all revisions
"master,feat/" --> master, branches matching feat/

mikejoh · 2023-12-01T13:04:38Z

Well, the whitelist is a good idea and we could switch off this feature if whitelist is empty.

Possible whitelist values:

"" --> switch off

"master" --> only master branch, other revisions are labelized as "other"

"master,develop"

"*" --> switch on, all revisions

"master,feat/" --> master, branches matching feat/

Yes, this makes sense, by coincidence I saw something similar in kube-state-metrics which exposes a --metric-labels-allowlist flag that does the following:

Comma-separated list of additional Kubernetes label keys that will be used in the resource' labels metric. By default the labels metrics are not exposed. To include them, provide a list of resource names in their plural form and Kubernetes label keys you would like to allow for them (Example: '=namespaces=[k8s-label-1,k8s-label-n,...],pods=[app],...)'. A single '' can be provided per resource instead to allow any labels, but that has severe performance implications (Example: '=pods=[]'). Additionally, an asterisk () can be provided as a key, which will resolve to all resources, i.e., assuming '--resources=deployments,pods', '==[]' will resolve to '=deployments=[],pods=[*]'.

Note that this flag adds another metric called kube_node_labels, which then can be used with e.g. joins in PromQL. In this case with ArgoCD, adding a separate metric might also be considered?

ebuildy · 2023-12-28T16:10:19Z

Well, the whitelist is a good idea and we could switch off this feature if whitelist is empty.
Possible whitelist values:

"" --> switch off

"master" --> only master branch, other revisions are labelized as "other"

"master,develop"

"*" --> switch on, all revisions

"master,feat/" --> master, branches matching feat/

Yes, this makes sense, by coincidence I saw something similar in kube-state-metrics which exposes a --metric-labels-allowlist flag that does the following:

Comma-separated list of additional Kubernetes label keys that will be used in the resource' labels metric. By default the labels metrics are not exposed. To include them, provide a list of resource names in their plural form and Kubernetes label keys you would like to allow for them (Example: '=namespaces=[k8s-label-1,k8s-label-n,...],pods=[app],...)'. A single '' can be provided per resource instead to allow any labels, but that has severe performance implications (Example: '=pods=[]'). Additionally, an asterisk () can be provided as a key, which will resolve to all resources, i.e., assuming '--resources=deployments,pods', '==[]' will resolve to '=deployments=[],pods=[*]'.

Note that this flag adds another metric called kube_node_labels, which then can be used with e.g. joins in PromQL. In this case with ArgoCD, adding a separate metric might also be considered?

Check latest commits!

Signed-off-by: ebuildy <[email protected]>

Signed-off-by: Thomas Decaux <[email protected]>

tibuntu · 2024-03-13T12:49:17Z

Any news on this?

antonhornquist · 2024-04-15T07:31:37Z

+1. any news on this PR? we'd really appreciate this support.

reegnz · 2024-04-15T14:50:20Z

controller/metrics/metrics.go

@@ -384,7 +395,7 @@ func (c *appCollector) collectApps(ch chan<- prometheus.Metric, app *argoappv1.A

 	autoSyncEnabled := app.Spec.SyncPolicy != nil && app.Spec.SyncPolicy.Automated != nil

-	addGauge(descAppInfo, 1, strconv.FormatBool(autoSyncEnabled), git.NormalizeGitURL(app.Spec.GetSource().RepoURL), app.Spec.Destination.Server, app.Spec.Destination.Namespace, string(syncStatus), string(healthStatus), operation)
+	addGauge(descAppInfo, 1, strconv.FormatBool(autoSyncEnabled), git.NormalizeGitURL(app.Spec.GetSource().RepoURL), c.normalizeAppTargetRevision(app.Spec.GetSource().TargetRevision), app.Spec.Destination.Server, app.Spec.Destination.Namespace, string(syncStatus), string(healthStatus), operation)


What would the metric show if targetRevision is not defined in the argocd Application?
You should add HEAD in case targetRevision is not defined (as that's the default).
Displaying an empty string is misleading and most people likely won't have an explicit targetRevision set, so they might be surprised.

For me as a user it would be VERY confusing to have master, main, develop whitelisted by default, and then seeing an empty string for the label, when argocd actually syncs from main. Setting it to HEAD is less confusing, it's the default if targetRevision is not set, and matches existing docs: https://argo-cd.readthedocs.io/en/stable/user-guide/tracking_strategies/#head-branch-tracking

reegnz · 2024-04-15T14:54:01Z

controller/metrics/metrics_test.go

-argocd_app_info{autosync_enabled="false",dest_namespace="dummy-namespace",dest_server="https://localhost:6443",health_status="Healthy",name="my-app",namespace="argocd",operation="",project="important-project",repo="https://github.com/argoproj/argocd-example-apps",sync_status="Synced"} 1
-argocd_app_info{autosync_enabled="true",dest_namespace="dummy-namespace",dest_server="https://localhost:6443",health_status="Healthy",name="my-app-2",namespace="argocd",operation="sync",project="important-project",repo="https://github.com/argoproj/argocd-example-apps",sync_status="Synced"} 1
+argocd_app_info{autosync_enabled="true",dest_namespace="dummy-namespace",dest_server="https://localhost:6443",health_status="Degraded",name="my-app-3",namespace="argocd",operation="delete",project="important-project",repo="https://github.com/argoproj/argocd-example-apps",revision="develop",sync_status="OutOfSync"} 1
+argocd_app_info{autosync_enabled="false",dest_namespace="dummy-namespace",dest_server="https://localhost:6443",health_status="Healthy",name="my-app",namespace="argocd",operation="",project="important-project",repo="https://github.com/argoproj/argocd-example-apps",revision="",sync_status="Synced"} 1


I think showing an empty string here is misleading, and should show HEAD instead.
Then at least we can say empty string means something that's both not whitelisted branch and not the default HEAD branch.

Comparing my-app and my-app-4:

my-app the user didn't explicitly define a targetRevision, so it's really master or main or whatever the default branch is configured to. This should show HEAD as the metric label.
my-app-4 the user didn't explicitly whitelist the custom branch name so the label is empty. This is fine.

ebuildy · 2024-04-15T21:32:47Z

hello @reegnz , thanks you for the feedbacks! I have edited the PR to handle "head" .

reegnz · 2024-04-16T08:49:38Z

controller/metrics/metrics.go

@@ -363,6 +363,9 @@ func boolFloat64(b bool) float64 {
 }

 func (c *appCollector) normalizeAppTargetRevision(in string) string {
+	if len(in) == 0 {
+		in = "head"


nitpicking, but git semantics seem to be using all caps HEAD, not lowercase. lowercase head is probably nothing special for git (unless you have a branch called head). At least that's what I'm seeing in all the docs and also ArgoCD UI, uppercase HEAD.

I have hesitated, meanwhile it's good to have prom labels lowercased right?

cmd/argocd-application-controller/commands/argocd_application_controller.go

Co-authored-by: Mikael Johansson <[email protected]> Signed-off-by: Thomas Decaux <[email protected]>

mikejoh · 2024-04-19T11:35:57Z

@ebuildy There were two variables with a typo:

metricsAplicationLabels
metricsAplicationRevisions

You fixed the latter one, but there's references to them when the command flags are composed in the same file. That seems to be the reason for the failed builds on the latest commit!

antonhornquist · 2024-04-25T13:37:00Z

AFAICT still typos r(metricsAplicationLabels), even thought these may be in the codebase since before does it make sense fixing them?

ebuildy · 2024-04-28T18:38:39Z

AFAICT still typos r(metricsAplicationLabels), even thought these may be in the codebase since before does it make sense fixing them?

Well, this is here from 3 years now ^^ https://github.com/argoproj/argo-cd/pull/7374/files#diff-21d1e1ae7561765d523f51ae68a0b94580d1a5980378aa8ccbaa2a7581fc0643R52 , I could do another PR later to fix it with pleasure!

jace-ys · 2024-08-10T16:36:44Z

Any updates on this? Would love to see this merged!

reegnz · 2024-08-12T15:51:01Z

AFAICT @ebuildy needs to sign off his commits because the DCO check is failing.
I'm also very eager to get this merged!

jenting · 2024-09-26T12:30:08Z

Would love to see this PR get reviewed and merged.

ebuildy force-pushed the feat_controller_app_info_metrics branch from 27a3b50 to 0f21375 Compare August 22, 2023 11:58

hoaraujerome approved these changes Aug 31, 2023

View reviewed changes

ashishkcs approved these changes Sep 5, 2023

View reviewed changes

tsouloumiacpetal approved these changes Nov 30, 2023

View reviewed changes

ebuildy force-pushed the feat_controller_app_info_metrics branch from 3343955 to a5bada1 Compare December 27, 2023 21:06

ebuildy requested a review from a team as a code owner December 27, 2023 21:06

ebuildy requested a review from a team as a code owner December 29, 2023 15:23

ebuildy force-pushed the feat_controller_app_info_metrics branch from 77e1348 to a7f6844 Compare December 29, 2023 15:28

ebuildy mentioned this pull request Jan 7, 2024

Add target_revision label to the argocd_app_info metric #16469

Closed

feat(controller): add targetRevision in metric app_info

5f956e3

Signed-off-by: ebuildy <[email protected]>

ebuildy force-pushed the feat_controller_app_info_metrics branch from 379cde4 to 13a5110 Compare January 7, 2024 23:48

ebuildy marked this pull request as draft January 7, 2024 23:51

ebuildy force-pushed the feat_controller_app_info_metrics branch from 13a5110 to 1afb96b Compare January 7, 2024 23:52

add metrics-app-revisions cli option

56b1bcd

Signed-off-by: ebuildy <[email protected]>

ebuildy force-pushed the feat_controller_app_info_metrics branch from 1afb96b to 56b1bcd Compare January 7, 2024 23:54

ebuildy marked this pull request as ready for review January 7, 2024 23:54

Merge branch 'master' into feat_controller_app_info_metrics

19c8572

ebuildy mentioned this pull request Jan 26, 2024

Add TargetRevision to argocd_app_info metric #4115

Open

Merge branch 'master' into feat_controller_app_info_metrics

2449b16

Signed-off-by: Thomas Decaux <[email protected]>

reegnz reviewed Apr 15, 2024

View reviewed changes

merge master

48170bb

ebuildy marked this pull request as draft April 15, 2024 20:07

feat: use head as default value

c3e4dbb

ebuildy marked this pull request as ready for review April 15, 2024 21:32

reegnz reviewed Apr 16, 2024

View reviewed changes

ebuildy added 2 commits April 16, 2024 08:45

feat: HEAD uppercase

c5563e3

feat: HEAD uppercase

dd0cb84

mikejoh reviewed Apr 18, 2024

View reviewed changes

cmd/argocd-application-controller/commands/argocd_application_controller.go Outdated Show resolved Hide resolved

Fix typo

9b817f1

Co-authored-by: Mikael Johansson <[email protected]> Signed-off-by: Thomas Decaux <[email protected]>

fix typos

9725651

ebuildy mentioned this pull request Apr 24, 2024

feat(metrics): git checkout failure counter (issue #15658) #15657

Merged

13 tasks

merge master

1c62bb1

ebuildy marked this pull request as draft April 28, 2024 19:11

ebuildy added 2 commits April 28, 2024 15:12

rollback makefile changes

6514f66

rollback unrelated fix

9b7215d

ebuildy marked this pull request as ready for review May 6, 2024 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(controller): add targetRevision in metric app_info #15143

feat(controller): add targetRevision in metric app_info #15143

ebuildy commented Aug 21, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

ashishkcs left a comment

ashishkcs commented Sep 5, 2023

sosoriov commented Oct 4, 2023

aspyker commented Oct 15, 2023

mikejoh commented Nov 29, 2023

ebuildy commented Nov 29, 2023

mikejoh commented Nov 30, 2023

ebuildy commented Nov 30, 2023

mikejoh commented Dec 1, 2023 •

edited

Loading

ebuildy commented Dec 28, 2023

tibuntu commented Mar 13, 2024

antonhornquist commented Apr 15, 2024

reegnz Apr 15, 2024 •

edited

Loading

reegnz Apr 15, 2024

ebuildy commented Apr 15, 2024

reegnz Apr 16, 2024 •

edited

Loading

ebuildy Apr 16, 2024

mikejoh commented Apr 19, 2024 •

edited

Loading

antonhornquist commented Apr 25, 2024

ebuildy commented Apr 28, 2024

jace-ys commented Aug 10, 2024 •

edited

Loading

reegnz commented Aug 12, 2024

jenting commented Sep 26, 2024

feat(controller): add targetRevision in metric app_info #15143

Are you sure you want to change the base?

feat(controller): add targetRevision in metric app_info #15143

Conversation

ebuildy commented Aug 21, 2023 • edited Loading

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

ashishkcs left a comment

Choose a reason for hiding this comment

ashishkcs commented Sep 5, 2023

sosoriov commented Oct 4, 2023

aspyker commented Oct 15, 2023

mikejoh commented Nov 29, 2023

ebuildy commented Nov 29, 2023

mikejoh commented Nov 30, 2023

ebuildy commented Nov 30, 2023

mikejoh commented Dec 1, 2023 • edited Loading

ebuildy commented Dec 28, 2023

tibuntu commented Mar 13, 2024

antonhornquist commented Apr 15, 2024

reegnz Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

reegnz Apr 15, 2024

Choose a reason for hiding this comment

ebuildy commented Apr 15, 2024

reegnz Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

ebuildy Apr 16, 2024

Choose a reason for hiding this comment

mikejoh commented Apr 19, 2024 • edited Loading

antonhornquist commented Apr 25, 2024

ebuildy commented Apr 28, 2024

jace-ys commented Aug 10, 2024 • edited Loading

reegnz commented Aug 12, 2024

jenting commented Sep 26, 2024

ebuildy commented Aug 21, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

mikejoh commented Dec 1, 2023 •

edited

Loading

reegnz Apr 15, 2024 •

edited

Loading

reegnz Apr 16, 2024 •

edited

Loading

mikejoh commented Apr 19, 2024 •

edited

Loading

jace-ys commented Aug 10, 2024 •

edited

Loading