ServiceMonitor endpoints port and targetPort ambiguity

nrvnrvn commented 5 years ago

What did you do? We spent a huge amount of time trying to understand why we could not scrape a newly added service metrics until we discovered the following:

You have a container (as a part of a Pod controlled by deployment/statefulset/etc.) and you don't specify a container port because Exposing a port here gives the system additional information about the network connections a container uses, but is primarily informational as stated in the API docs.
You have got a service which covers the aforementioned container(s) and you haven't given a name to it because when you have a single port exposed you are allowed to omit the name.
You assume that ServiceMonitor watches the Services(according to this, Endpoints to be precise).
API docs state that:
- port is a Name of the service port this endpoint refers to. Mutually exclusive with targetPort.
- targetPort is a Name or number of the target port of the endpoint. Mutually exclusive with port.
You create a ServiceMonitor in which you set the targetPort equal to the number of your Service's port.
You get no metrics...

What did you expect to see? Metrics What did you see instead? Under which circumstances? Instead we discovered that ServiceMonitor's targetPort actually transforms to __meta_kubernetes_pod_container_port_number which means that Prometheus is told to scrape not the Endpoints resources but the Container resources. See https://github.com/coreos/prometheus-operator/blob/v0.28.0/pkg/prometheus/promcfg.go#L378-L399 This creates a lot of confusion... Environment

Prometheus Operator version: v0.28.0@sha256:62c5fe0246d88746001f071bfc764d3480dc1fc6516af52bde9d2416778dc843

Kubernetes version information:

Client Version: version.Info{Major:"1", Minor:"13", GitVersion:"v1.13.4", GitCommit:"c27b913fddd1a6c480c229191a087698aa92f0b1", GitTreeState:"clean", BuildDate:"2019-03-01T23:34:27Z", GoVersion:"go1.12", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"11+", GitVersion:"v1.11.7-gke.12", GitCommit:"06f08e60069231bd21bdf673cf0595aac80b99f6", GitTreeState:"clean", BuildDate:"2019-02-25T20:37:10Z", GoVersion:"go1.10.8b4", Compiler:"gc", Platform:"linux/amd64"}

Kubernetes cluster kind: GKE
Manifests:

---
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: test
  name: test
spec:
  selector:
    matchLabels:
      app: test
  template:
    metadata:
      labels:
        app: test
    spec:
      containers:
      # please replace this with something that actually serves some metrics in order to reproduce the issue
      - image: k8s.gcr.io/pause-amd64:3.1
        name: pause-amd64
---
apiVersion: v1
kind: Service
metadata:
  labels:
    app: test
  name: test
spec:
  ports:
  - port: 8000
  selector:
    app: test
---
apiVersion: v1
kind: Endpoints
metadata:
  labels:
    app: test
  name: test
subsets:
- addresses:
  - ip: 10.20.9.31
    nodeName: somenodename
    targetRef:
      kind: Pod
      name: test-555dbcb98f-7ds92
      namespace: test-prometheus
      resourceVersion: "81073517"
      uid: e6556903-5075-11e9-9584-4201c0a8000b
  ports:
  - port: 8000
    protocol: TCP
---
# ServiceMonitor lives in the same namespace as Prometheus and Prometheus Operator of course
apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  labels:
    app: test
  name: test
  namespace: monitoring
spec:
  endpoints:
  - interval: 15s
    path: /metrics
    targetPort: 8000
  namespaceSelector:
    any: true
  selector:
    matchLabels:
      app: test

Prometheus Operator Logs:

insert Prometheus Operator logs relevant to the issue here

Proposal

Documentation should be amended to clearly state how the []Endpoints' port and targetPort work.

Since transforming the targetPort to __meta_kubernetes_pod_container_port_number/__meta_kubernetes_pod_container_port_name has been there for a long while a lot of users rely on this and this should not be removed for backwards compatibility.

Instead __meta_kubernetes_endpoint_port_number should be taken into consideration as well.

P.S. While I was writing all the above I haven't yet dug deeply into Prometheus itself but at first glance it seems like this is an upstream issue since I don't see __meta_kubernetes_endpoint_port_number here

Two questions here then:

should we raise the issue with Prometheus about the Endpoint port number and adding support for it?
what were the initial considerations for using __meta_kubernetes_endpoint_port* instead of __meta_kubernetes_service_port_*? I assume this was done because endpoints contain actual target information but I still ask this question because ServiceMonitor as we assumed should have grabbed Port number/name from Services.

brancz commented 5 years ago

You're absolutely right, the documentation is faulty here. The targetPort field doesn't refer to the Services fields, it refers to the port name or port number defined on the underlying selected Pods. Clarifying this in the docs is step 1.

Step 2 as you also already mentioned, we should treat port as a IntOrStr just like targetPort and select either the port name or port number, in the same fashion. And in order to do that, Prometheus should expose the __meta_kubernetes_endpoint_port_number meta label. Would you like to go ahead and open this issue? If not I'm happy to do it, but as you discovered it, it's yours to take :slightly_smiling_face: .

what were the initial considerations for using __meta_kubernetes_endpoint_port instead of __meta_kubernetes_serviceport? I assume this was done because endpoints contain actual target information but I still ask this question because ServiceMonitor as we assumed should have grabbed Port number/name from Services.

This is because the Endpoints object is what Prometheus actually uses for discovery purposes, and a Service is not necessarily always present. So going with the Endpoints object whenever possible is the safe bet.