Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add metrics port to mgmt service #112

Merged
merged 1 commit into from
Dec 21, 2018

Conversation

arminbuerkle
Copy link

While adding prometheus annotations for metric scraping is good enough for the usual prometheus deployment, the prometheus operator requires a service target for its service monitor.

prometheus-operator/kube-prometheus#16 (comment)


This PR exposes the metrics port on the nats mgmt svc, which can then be scraped by a coreos ServiceMonitor:

---
apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  labels:
    app: nats
  name: nats
spec:
  endpoints:
    - interval: 30s
      port: metrics
  jobLabel: nats
  selector:
    matchLabels:
      app: nats

@@ -128,6 +128,12 @@ func CreateMgmtService(kubecli corev1client.CoreV1Interface, clusterName, cluste
TargetPort: intstr.FromInt(constants.MonitoringPort),
Protocol: v1.ProtocolTCP,
},
{
Name: "metrics",
Port: constants.MetricsPort,
Copy link
Author

Choose a reason for hiding this comment

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

It'll add a metrics port regardless whether metrics are enabled or not on the crd.

While this isn't optimal, the current code doesn't give the option to update the service after it is created. Meaning if someone enables metrics after deploying a nats cluster, it won't be updated.


Additionally AFAIK k8s doesn't add the pods to the service/metrics endpoint which do not expose their metrics pod.

Therefore if metrics are disabled, no endpoints will be added, which is the correct behaviour.

Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

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

LGTM

@wallyqs wallyqs merged commit 2ae9c6d into nats-io:master Dec 21, 2018
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.

2 participants