feat(controller): add targetRevision in metric app_info

[ebuildy]

Add "targetRevision" for argocd_app_info metric.

--> Should fix https://github.com/argoproj/argo-cd/issues/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[bot]]

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.

:exclamation: 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 :warning:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[ashishkcs]

@alexmt @jessesuen could you please approve this?

[sosoriov]

What is the current status of this?

[aspyker]

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

[mikejoh]

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]

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]

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]

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]

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]

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!

[tibuntu]

Any news on this?

[antonhornquist]

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

[ebuildy]

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

[mikejoh]

@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]

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

[ebuildy]

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]

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