banzaicloud / koperator

Oh no! Yet another Apache Kafka operator for Kubernetes
Apache License 2.0
784 stars 195 forks source link

Configurable prometheus operator annotations #398

Closed azun closed 4 years ago

azun commented 4 years ago

Is your feature request related to a problem? Please describe. Currently Kafka brokers and CruiseControl are monitored using prometheus annotations (prometheus.io/scrape=true). These annotations are hardcoded in the operator and offer limited options for configuration (lack of scrape interval/timeout configs, relabelling, isolation etc).

Describe the solution you'd like to see Ideally the annotations should be configurable by the user as an opt-in or opt-out

# opt-in
spec:
  brokerConfigGroups:
    default:
      brokerAnnotations:
        prometheus.io/port: 9020
        prometheus.io/scrape: true
  cruiseControlConfig:
    annotations:
      prometheus.io/port: 9020
      prometheus.io/scrape: true

# opt-out
spec:
  brokerConfigGroups:
    default:
      externalMonitoring: true
  cruiseControlConfig:
    externalMonitoring: true

ServiceMonitors would then be created separately

Describe alternatives you've considered N/A

Additional context The documentation actually instructs to create ServiceMonitors but doing so would duplicate the metrics in prometheus. Disabling annotation scraping would fix this but it's not desirable.

alungu commented 4 years ago

The solution we are proposing is to drop the hard coded Prometheus annotations and use the brokerAnnotations [1] property for the Prometheus annotations, too. We could have the Prometheus scraping annotations set as defaults. We could also use the same technique for the CC monitoring. [1] https://github.com/banzaicloud/kafka-operator/blob/master/charts/kafka-operator/templates/operator-kafka-crd.yaml#L62

What do you guys thing? I could open a PR with the implementation, as well.

baluchicken commented 4 years ago

I really like the idea to remove this hard coded Prometheus annotations. It already come to our mind as well since the Prometheus Operator does not require these annotations as well. We also like the idea to reuse the already available brokerAnnotations field. Although the operator should also open a metrics port in the annotation specified port, now that part is also hard coded. We suggest to remove that part as well and let the user decide which port should be opened for metrics. Two options came into my mind:

What do you guys think?

amuraru commented 4 years ago

hi @baluchicken - thanks for looking at this

decide which port should be opened for metrics.

I don't think we have an issue here, kafka pod already defines a metrics port:

       - name: metrics
          containerPort: 9020
          protocol: TCP

and a Prometheus ServiceMonitor can refer this named port: https://github.com/banzaicloud/kafka-operator/blob/5f78e61389c7bdee7a7d03664f345b0ca3db0796/config/samples/kafkacluster-prometheus.yaml#L10-L20

so there is no need to keep the prometheus.io/port annotation either

baluchicken commented 4 years ago

Thanks after internal discussion we agreed that the proposed solution is good. Can you please do the PR for this? Thanks.

alungu commented 4 years ago

Thanks after internal discussion we agreed that the proposed solution is good. Can you please do the PR for this? Thanks.

Sure! I'll open a PR with the fix on the following days. Thanks!

bechhansen commented 3 years ago

Hi @alungu and @baluchicken

After reading this my expectation is that the operator will never by default set any prometheus annotations on Kafka brokers pods. Is this correct?

The documentation here state the opposite: https://banzaicloud.com/docs/supertubes/kafka-operator/monitoring/

By default operator installs Kafka Pods with the following annotations, also it opens port 9020 in all brokers to enable scraping. "prometheus.io/scrape": "true" "prometheus.io/port": "9020"

stoader commented 3 years ago

@bechhansen correct, kafka-operator only sets annotations on the broker pod specified in the KafkaCluster CR.

See: https://github.com/banzaicloud/kafka-operator/blob/master/pkg/resources/kafka/pod.go#L106

bechhansen commented 3 years ago

@bechhansen correct, kafka-operator only sets annotations on the broker pod specified in the KafkaCluster CR.

See: https://github.com/banzaicloud/kafka-operator/blob/master/pkg/resources/kafka/pod.go#L106

Thanks for that clarification @stoader.

That's three days of wasted unnecessary debugging I will never get back. I guess the documentation need to bed updated?

Also I think the opt-in example written her is wrong. It should be:

spec: brokerConfigGroups: default: brokerAnnotations: prometheus.io/port: "9020" prometheus.io/scrape: "true" cruiseControlConfig: cruiseControlAnnotations: prometheus.io/port: "9020" prometheus.io/scrape: "true"