Skip to content

Conversation

@dkarpele
Copy link
Collaborator

@dkarpele dkarpele commented Sep 19, 2025

Prometheus metrics were successfully exported after refactoring

dkarpele@dkarpele-mac /tmp % curl --insecure --header "Authorization: Bearer ${TOKEN}" https://127.0.0.1:8443/metrics | grep argocd
argocd_image_updater_applications_watched_total The total number of applications watched by Argo CD Image Updater CR
# TYPE argocd_image_updater_applications_watched_total gauge
argocd_image_updater_applications_watched_total{image_updater_cr_name="image-updater-001",image_updater_cr_namespace="argocd-image-updater-system"} 1
# HELP argocd_image_updater_k8s_api_errors_total The total number of K8S API requests resulting in error
# TYPE argocd_image_updater_k8s_api_errors_total counter
argocd_image_updater_k8s_api_errors_total 0
# HELP argocd_image_updater_k8s_api_requests_total The total number of K8S API requests performed by the Argo CD Image Updater
# TYPE argocd_image_updater_k8s_api_requests_total counter
argocd_image_updater_k8s_api_requests_total 0
  • Remove legacy flags health-port and metrics-port and replace them with health-probe-bind-address and metrics-bind-address. Update docs.
  • health module was removed in favor of controller-runtime.
  • Add RBAC rules for metrics.
  • Images metrics will be implemented in GITOPS-8068

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2025

Codecov Report

❌ Patch coverage is 82.22222% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.55%. Comparing base (9d406c4) to head (a7a7456).
⚠️ Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
cmd/run.go 33.33% 6 Missing ⚠️
cmd/common.go 0.00% 1 Missing ⚠️
pkg/webhook/server.go 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1243      +/-   ##
==========================================
+ Coverage   70.34%   70.55%   +0.21%     
==========================================
  Files          50       49       -1     
  Lines        4461     4449      -12     
==========================================
+ Hits         3138     3139       +1     
+ Misses       1130     1117      -13     
  Partials      193      193              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dkarpele dkarpele force-pushed the dk-GITOPS-7683 branch 6 times, most recently from ee686a1 to 2d26552 Compare October 27, 2025 19:23
@dkarpele dkarpele marked this pull request as ready for review October 27, 2025 19:30
@dkarpele dkarpele self-assigned this Oct 27, 2025
@chengfang
Copy link
Collaborator

For the metric port and health port flags, shall we also support setting them via env var? If so, also update manager.yaml?

The new flag names, health-probe-bind-address, metrics-bind-address, look like support both hostname or ip plus port number. But I think in practice, we only use the port number part. What are the accepted format, are values like :9999 or 9999 both acceptable?

I'm wondering if we should rename them from the old health-port and metrics-port, or just stick to the old names. The old names are consistent with argo-cd (for example, https://github.com/argoproj/argo-cd/blob/master/cmd/argocd-server/commands/argocd_server.go#L314), and a bit more readable. If we only care about the port part, I feel the old names are fine.

cmd/run.go Outdated
// TODO: flags below are not documented yet and don't have env vars yet. Metrics and health checks will be implemented in GITOPS-7113
controllerCmd.Flags().StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. Use :8443 for HTTPS or :8080 for HTTP, or leave as 0 to disable the metrics service.")
controllerCmd.Flags().StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
controllerCmd.Flags().StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to. Leave as 0 to disable the probe service.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

the default is :8081, so 'Leave as 0' can be confusing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How does the health-probe-bind-address work with the liveness setting in manager.yaml? Are there overlapping, or should they be kept in sync?

        livenessProbe:
          httpGet:
            path: /healthz
            port: 8081

Copy link
Collaborator Author

@dkarpele dkarpele Oct 29, 2025

Choose a reason for hiding this comment

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

They should be kept in sync. If these settings are not synchronized, the health check will fail, leading to the pod being terminated and restarted. health-probe-bind-address tells the controller which port to listen on for incoming health check requests. livenessProbe is a k8s setting that tells the kubelet where to send requests to check if the container is healthy. If they don't match the health check fails, connection is refused and k8s kills the pod.

@dkarpele dkarpele changed the title feat(crd): Refactor prometheus metrics feat: Refactor prometheus metrics Oct 29, 2025
@dkarpele
Copy link
Collaborator Author

For the metric port and health port flags, shall we also support setting them via env var? If so, also update manager.yaml?

We didn't have env vars before for these flags, argocd doesn't have env vars for metrics port (see link you provided), metrics are optional and disabled by default (it's 0). So I think we shouldn't add them as env vars at least before customers request.

@dkarpele
Copy link
Collaborator Author

dkarpele commented Oct 29, 2025

The new flag names, health-probe-bind-address, metrics-bind-address, look like support both hostname or ip plus port number. But I think in practice, we only use the port number part. What are the accepted format, are values like :9999 or 9999 both acceptable?
I'm wondering if we should rename them from the old health-port and metrics-port, or just stick to the old names. The old names are consistent with argo-cd (for example, https://github.com/argoproj/argo-cd/blob/master/cmd/argocd-server/commands/argocd_server.go#L314), and a bit more readable. If we only care about the port part, I feel the old names are fine.

The accepted formats are:
":8081"
"host.com:8081"
"10.20.30.40:8081"
not recommended is "8081"

The difference between argocd flags and image updater flags is that argocd flag is int but we use string. Yes, in practice we use only port numbers but library supports hostnames and ip addresses, there are options BindAddress and HealthProbeBindAddress. If we stick to the old names it makes things even more confusing: the field name metrics-port accept hostname/ip + port.

health-probe-bind-address, metrics-bind-address flag names were autogenerated by kubebuilder.

Co-authored-by: Gemini AI <[email protected]>"
Signed-off-by: Denis Karpelevich <[email protected]>
@chengfang chengfang merged commit 950af4b into argoproj-labs:master Oct 29, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants