Closed invidian closed 7 months ago
DISCLAIMER; I do not mean to bash on the code which is written or on the person who wrote the code. It is only meant as "what should be done to allow for an "easier" path of implementing Protocol Buffers" and not in any way as criticism.
(metricshandler.MetricsHandler).ServeHTTP
is responsible for serving HTTP requests for metrics. Though metricshandler.MetricsHandler
is not only responsible for writing metrics to the response (to be a bit more specific than "serving HTTP requests") but as well for sharding and applying compression (GZIP).
https://github.com/kubernetes/kube-state-metrics/blob/master/pkg/metricshandler/metrics_handler.go
I think this does too much and should be refactored before even trying to implement Protocol Buffers.
Sharding should be refactored outside of the HTTP handler, such that the HTTP handler is only aware of the metrics that should be written.
GZIP compression should be "decorated" onto the the HTTP handler or http.ResponseWriter
before or after it is passed into (metricshandler.MetricsHandler).ServeHTTP
, thus taking away the responsibility of applying compression away from the HTTP handler.
Though this goes further into (metricsstore.MetricsWriter).WriteAll
which makes assumptions about the data format as well by manually writing \n
to the response after the "HELP" of a metric.
In metricsstore.MetricsStore
the metrics are already kept as a multi-dimensional byte array ([][]byte
). Though I don't know whether this is a problem for implementing Protocol Buffers or not.
https://github.com/kubernetes/kube-state-metrics/blob/master/pkg/metrics_store/metrics_store.go#L39
//cc @fpetkovski what do you think? In terms of the specific arguments I made about the code that should be refactored but as well about the approach of doing a refactor before implementing Protocol Buffers to keep it small and manageable.
A refactor would not only be about separating responsibilities but also be a bit of thinking ahead in terms of what abstractions to put in place to allow for easier implementation of Protocol Buffers in the future (or other formats for that matter).
I asked this in Slack as well but will ask it on here too simply so that everything and every question / answer is kept within the issue and doesn't get lost 👍🏼
I was thinking / prototyping a bit to implement Protocol Buffers in KSM and a simple question arose with maybe a simple answer but I don't know.
Why hasn't KSM used the Go client library by Prometheus to implement its metrics? The question arose because the client seems to already support Protocol Buffers.
https://kubernetes.slack.com/archives/CJJ529RUY/p1640638803044600
Thanks for looking into it @Serializator!
Why hasn't KSM used the Go client library by Prometheus to implement its metrics?
I forgot to mention that in the opening post. I think that would definitely be a preferable solution for this issue!
From @fpetkovski
The main reason for this is because KSM dumps a lot of metrics, especially in large clusters, and using the go client library has proven to be slow and memory intensive in the past.
This might be a useful read to get a bit more context https://github.com/prometheus/client_golang/discussions/917
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
lifecycle/stale
is appliedlifecycle/stale
was applied, lifecycle/rotten
is appliedlifecycle/rotten
was applied, the issue is closedYou can:
/remove-lifecycle stale
/lifecycle rotten
/close
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
/remove-lifecycle stale
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
lifecycle/stale
is appliedlifecycle/stale
was applied, lifecycle/rotten
is appliedlifecycle/rotten
was applied, the issue is closedYou can:
/remove-lifecycle stale
/lifecycle rotten
/close
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
/remove-lifecycle stale
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
lifecycle/stale
is appliedlifecycle/stale
was applied, lifecycle/rotten
is appliedlifecycle/rotten
was applied, the issue is closedYou can:
/remove-lifecycle stale
/lifecycle rotten
/close
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
/remove-lifecycle stale
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
lifecycle/stale
is appliedlifecycle/stale
was applied, lifecycle/rotten
is appliedlifecycle/rotten
was applied, the issue is closedYou can:
/remove-lifecycle stale
/lifecycle rotten
/close
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
/remove-lifecycle stale
The Kubernetes project currently lacks enough contributors to adequately respond to all issues.
This bot triages un-triaged issues according to the following rules:
lifecycle/stale
is appliedlifecycle/stale
was applied, lifecycle/rotten
is appliedlifecycle/rotten
was applied, the issue is closedYou can:
/remove-lifecycle stale
/close
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
/remove-lifecycle stale
The Kubernetes project currently lacks enough contributors to adequately respond to all issues.
This bot triages un-triaged issues according to the following rules:
lifecycle/stale
is appliedlifecycle/stale
was applied, lifecycle/rotten
is appliedlifecycle/rotten
was applied, the issue is closedYou can:
/remove-lifecycle stale
/close
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
/remove-lifecycle stale
The Kubernetes project currently lacks enough contributors to adequately respond to all issues.
This bot triages un-triaged issues according to the following rules:
lifecycle/stale
is appliedlifecycle/stale
was applied, lifecycle/rotten
is appliedlifecycle/rotten
was applied, the issue is closedYou can:
/remove-lifecycle stale
/close
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.
This bot triages un-triaged issues according to the following rules:
lifecycle/stale
is appliedlifecycle/stale
was applied, lifecycle/rotten
is appliedlifecycle/rotten
was applied, the issue is closedYou can:
/remove-lifecycle rotten
/close
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.
This bot triages issues according to the following rules:
lifecycle/stale
is appliedlifecycle/stale
was applied, lifecycle/rotten
is appliedlifecycle/rotten
was applied, the issue is closedYou can:
/reopen
/remove-lifecycle rotten
Please send feedback to sig-contributor-experience at kubernetes/community.
/close not-planned
@k8s-triage-robot: Closing this issue, marking it as "Not Planned".
What would you like to be added:
kube-apiserver and kubelet (and probably other core Kubernetes components) supports scraping Prometheus metrics using protobuf:
KSM only accepts plaintext protocol though:
Protobuf definitely offers smaller network traffic, as number of data transferred varies between 3-7 times less from quick testing in favor of protobuf. I've seen #498 issue, but I couldn't find anything related to this issue. Possibly protobuf encoding is also more CPU efficient?
Why is this needed:
To make KSM consume less resources and to make it more aligned with core Kubernetes components.
Additional context
Discovered as part of work on https://github.com/newrelic/nri-kubernetes/issues/234.