Adding ClusterIP service in helm chart to expose controller metrics.

[vijtrip2]

Description of changes:

Adding ClusterIP service in helm chart to expose controller metrics.

It is much simpler and easier to update prometheus scraping configuration with a static endpoint.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[a-hilaly]

Great! 👍

[RedbackThomson]

I don't think this is necessary for any of the default installations we offer. There is no reason to access the controller pods through a service as they offer no network APIs. I would rather see a separate service spec added to cluster gitops alongside the helm chart if a customer wants to expose metrics in this way.

If this is a must, then at least gating the service behind a create: true/false in the helm values would be required. (Default being false)

[a-hilaly]

I don't think this is necessary for any of the default installations we offer. There is no reason to access the controller pods through a service as they offer no network APIs. I would rather see a separate service spec added to cluster gitops alongside the helm chart if a customer wants to expose metrics in this way.

There is a metrics api exposed at :8080 in every controller https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/config/config.go#L60-L64

[a-hilaly]

If this is a must, then at least gating the service behind a create: true/false in the helm values would be required. (Default being false)

LGTM, If we're going with solution, we could also gate the metrics API using service controller flags, thinking --enable-metrics-server

[RedbackThomson]

I don't think this is necessary for any of the default installations we offer. There is no reason to access the controller pods through a service as they offer no network APIs. I would rather see a separate service spec added to cluster gitops alongside the helm chart if a customer wants to expose metrics in this way.

There is a metrics api exposed at :8080 in every controller https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/config/config.go#L60-L64

I understand that, but does Prometheus not detect all metrics servers in every pod by default?

[vijtrip2]

"If this is a must, then at least gating the service behind a create: true/false in the helm values would be required. (Default being false)"

Good Idea. I will definitely do this.
[Edit:] Done.

"I understand that, but does Prometheus not detect all metrics servers in every pod by default?"

While prototyping, I did not find this to be the case. I can double check.
a) First I tried to use prometheus Kubernetes Service Discovery for finding these metrics but was not able to successfully do it.
b) Then I tried this static config method where we ask Prometheus to scrape the metric from a static endpoint. It was very simple, straight forward and clean.
Hence I went ahead with method (b).

[RedbackThomson]

/lgtm

[ack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, RedbackThomson, vijtrip2

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [A-Hilaly,RedbackThomson,vijtrip2]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[a-hilaly]

I understand that, but does Prometheus not detect all metrics servers in every pod by default?

AFAIK Prometheus does not detect any metrics at all by default - it needs to be configured for pod discovery

[jaypipes]

@vijtrip2 @RedbackThomson @a-hilaly

Can we rename this to specify that it has to do with the metrics server and not the service controller itself?

Perhaps something like this?

metrics:
  service:
    # Set to true to automatically create a Kubernetes Service resource for the
    # Prometheus metrics server endpoint in controller
    create: false
    # Which Type to use for the Kubernetes Service?
    # See: https://kubernetes.io/docs/concepts/services-networking/service/#publishing-services-service-types
    type: ClusterIP

[vijtrip2]

Yep, I will get it done.

[jaypipes]

Can we rename this file to templates/metrics-service.yaml please?