search

hashicorp / consul-k8s

First-class support for Consul Service Mesh on Kubernetes
https://www.consul.io/docs/k8s
Mozilla Public License 2.0
667 stars 318 forks source link

helm:Add Prometheus Service Monitor Operator support #671

Open sergeyshaykhullin opened 4 years ago

sergeyshaykhullin commented 4 years ago

Add Prometheus operator ServiceMonitor support like:

  serviceMonitor:
    enabled: true
sergeyshaykhullin commented 3 years ago

@ishustava Does this issue is in someone's roadmap?

I can create PR with ServiceMonitor

It will monitor service/consul-server:8500/v1/agent/metrics?format=prometheus I don't get how to use client metrics, does it has separate one, or consul-server is returning all? P.S. As i understand it is returning all metrics required, so we have to have one root ServiceMonitor just for it?

How it might look like

server:
  extraConfig: |
    {"telemetry": {"prometheus_retention_time": "120s"}}

client:
  extraConfig: |
    {"telemetry": {"prometheus_retention_time": "120s"}}

serviceMonitor:
  enabled: true
  interval: ...
  scrapeTimeout: ...
  namespace: ...
lkysow commented 3 years ago

Hi Sergey,

I don't get how to use client metrics.

Some metrics only need to be retrieved from the consul servers but there are other metrics that both clients and servers have, e.g. consul.api.http, that would likely also be useful to collect across all clients and servers.

With the upcoming Consul 1.9 release, we are adding the ability to view prometheus metrics in the Consul UI so we will be looking at deeper prometheus support in the helm chart. So if you have a serviceMonitor config that you recommend we'd love to see a PR.

kong62 commented 3 years ago

+1

thisisnotashwin commented 3 years ago

Related to hashicorp/consul-helm#262

danfromtitan commented 3 years ago

With the upcoming Consul 1.9 release, we are adding the ability to view prometheus metrics in the Consul UI so we will be looking at deeper prometheus support in the helm chart. So if you have a serviceMonitor config that you recommend we'd love to see a PR.

Curious where the future lies with Prometheus Operator integration. So far Consul metrics diverges from what this issue was trying to address in the (now closed) https://github.com/hashicorp/consul-helm/pull/730

The issue with the later metrics is that it's too opinionated on how metrics should be retrieved and it looses sights on how the Prometheus community consumes those metrics.

Seriously, who cares about viewing metrics in Consul's UI ? At the same time pulling metrics from TLS-enabled endpoints is not supported. Prometheus Operator would just help not having to re-invent the wheels of pulling metrics :)

lkysow commented 3 years ago

This is still on our roadmap to address, we just haven't had the time to get to it. We agree that the current implementation has some major gaps.

ndhanushkodi commented 2 years ago

@hamishforbes opened a related issue (#973) and I'm pasting the contents here to keep a single thread for discussion.

Is your feature request related to a problem? Please describe.

The current metrics feature is not easily integrated with prometheus-operator based prometheus deployments like kube-prometheus

The current implementation only adds prometheus.io/* annotations to the pod to allow prometheus to discover scrape targets. While you could add custom configuration to a prometheus-operator based deployment to pickup these annotations, they're not supported by default.

Instead the prometheus operator uses custom resources (ServiceMonitor and PodMonitor) to configure targets for scraping.

However, referring to target ports by their integer value is a deprecated feature, ports should be referenced by their name instead (see targetPort).

So with the current implementation I cannot create ServiceMonitor or PodMonitor resources to scrape the sidecar metrics, without adding a named port to my application container. Although this works it's not ideal, the named port is defined on the wrong container and it would just be cleaner if consul-k8s could configure the ports correctly when injecting the sidecar.

The annotations are also superfluous noise in this setup.

Feature Description

I'm suggesting 2 changes here:

  • Add named ports to the envoy-sidecar container
  • Add a flag to disable injecting the prometheus.io annotations
fbozic commented 2 years ago

Doing my part in this request, I would really appreciate this feature. Is there any ETA for this?

hamishforbes commented 2 years ago

If you don't mind running a fork I have a patch that names the envoy port and disables the legacy annotations.

You can then at least create your own PodMonitor or ServiceMonitor resources

david-yu commented 2 years ago

@hamishforbes would you be able to create a PR that would somehow help us support Prometheus Service Monitor Operator?

hamishforbes commented 2 years ago

@david-yu I can probably find some time to open a PR for at least part of it this week Theres 2 parts as I see it

1) Adding ServiceMonitor resources to the Helm chart to configure scraping of the various Consul components (i.e. wherever the Helm chart currently adds prometheus.io/scrape annotations)

2) Fixing the injected Envoy container to use named ports so users don't have to use deprecated functionality when creating their own ServiceMonitor/PodMonitor resources for application pods

3) Optionally removing the prometheus.io/* annotations, configurable obviously, I'm sure some people will want to use those