keps/sig-instrumentation: Add metrics overhaul KEP #2909
Conversation
k8s-ci-robot
added
the
size/L
|
@brancz: GitHub didn't allow me to request PR reviews from the following users: metalmatze, s-urbaniak, mxinden. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
kind/kep
sig/architecture
|
|
||
| A number of metrics that Kubernetes is instrumented with do not follow the [official Kubernetes instrumentation guidelines](https://github.com/kubernetes/community/blob/master/contributors/devel/instrumentation.md). This is for a number of reasons, such as the metrics having been created before the instrumentation guidelines were put in place (around two years ago), and just missing it in code reviews. Beyond the Kubernetes instrumentation guidelines, there are several violations of the [Prometheus instrumentation best practices](https://prometheus.io/docs/practices/instrumentation/). In order to have consistently named and high quality metrics, this effort aims to make working with metrics exposed by Kubernetes consistent with the rest of the ecosystem. In fact even metrics exposed by Kubernetes are inconsistent in themselves, making joining of metrics difficult. | ||
|
|
||
| Kubernetes also makes extensive use of a global metrics registry to register metrics to be exposed. Aside from general shortcomings of global variables, Kubernetes is seeing actual effects of this, causing a number of components to use `sync.Once` or other mechanisms to ensure to not panic, when registering metrics. Instead a metrics registry should be passed to each component in order to explicitly register metrics instead of through `init` methods or other global, non-obvious executions. |
There was a problem hiding this comment.
My worry with this is that it will lead to excuses for not registering metrics -- e.g. "oh, I don't want to plumb the registry through to this individual function, so I guess I'll just skip adding a metric here". I'm not necessarily entirely against it, but I think the potential cons deserve exploration.
FWIW, we took the approach of having a separate global registry in controller-runtime, so you do metrics.Registry.MustRegister instead of prometheus.MustRegister, but afterwards the workflow feels similar. This lets still lets people choose to separate out the metrics, but doesn't require the extra plumbing.
I'd be curious to see an exploration of the sync.Once -- I seem to recall a decent amount of that was around the idea of metrics being optional, or having optional providers. Perhaps we should just be opinionated about that?
There was a problem hiding this comment.
While it is probably true that people are going to be tempted to not pass down the registry and thus avoid adding metrics in the first place, having another global registry only avoids the overall problem to some degree.
If somebody uses code of the controller-runtime as a library the metrics of that package are still going to be registered against that registry without any explicit configuration. Therefore people might not be aware of that fact and either think that the metrics should show up, but they don't (because they don't use the registry's handler) or due some other mistakes might still show up even though the are writing a completely different component.
We've seen these things happening too often...
Explicitly passing the registry avoids a lot of headache in this case.
There was a problem hiding this comment.
My own opinion: by introducing a new global registry we actually get the worst of both worlds. It doesn't allow us to capture the metrics that are already being registered by libraries against the canonical prometheus.DefaultRegisterer, and we must create additional work-arounds to collect all metrics as we're forced to have multiple endpoints or even ports. I don't see why the controller-runtime in particular can't inject the registry and default to the canonical global one (similar to the go http client).
Excusing global state with it being easier is a poor argument to make I think, we don't start introducing global variables all over the place for a reason, and it should be the same for metrics/logging/tracing. Plus all of those signals get better by having them plumbed through as that allows us to add extra context.
I think this discussion boils down to any global vs. non-global (dependency injection) discussion, and I think that regardless of metrics/logging/tracing, we can all agree that globals are harmful, or at least encourage harmful patterns.
There are nice abstractions in client-go to make metrics optional, I'd like to see that being explored further as well. I think in terms of this KEP it's more a nice to have, the main goal is that the metrics names don't keep changing, the plumbing can be improved afterwards I think. It's a legacy to work with, but that doesn't mean we can't improve it, in my opinion 🙂 .
There was a problem hiding this comment.
My own opinion: by introducing a new global registry we actually get the worst of both worlds. It doesn't allow us to capture the metrics that are already being registered by libraries against the canonical prometheus.DefaultRegisterer
The thought was basically that you could plumb it through as an additional collector if you wanted to merge with another registry. I recall going through and figuring out what was required, and it seemed fairly straightforward to serve multiple collectors from a single endpoint (I think).
Plus all of those signals get better by having them plumbed through as that allows us to add extra context.
Sure, agreed. I think we just have to be careful not to make plumbing all that a pain.
I think this discussion boils down to any global vs. non-global (dependency injection) discussion, and I think that regardless of metrics/logging/tracing, we can all agree that globals are harmful, or at least encourage harmful patterns.
I'll agree with that sentiment (on globals).
I think the key for me is that it should have friendly UX while still having sound architectural design -- adding a metric shouldn't necessarily requiring plumbing down a handle to a registry through 5 layers of objects, and updating 20 different test constructions of the intermediate structs that don't use the constructor method and therefore don't get the registry injected, causing the tests to panic (I've had to do similar things before ;-) ). Kubernetes is a twisted maze of structures, and doing changes to one often causes a cascade effect.
I look at it kind-of of like middleware -- you can layer on middleware without having to make much of a change to the core functionality to your web app. In a perfectly ideal world, IMO, you should should be able to "layer in" metrics/logging/etc without causing major ripples in your codebase. It should be there when you want it, and not there when you don't. Of course, we don't have a perfect world, but I think it's something to thing about closely.
🚲 🏠 I've got some other thoughts on workarounds (e.g. our logging promises in controller-runtime), but we can discuss those when the time comes.
There are nice abstractions in client-go to make metrics optional, I'd like to see that being explored further as well.
On this point I disagree. I think once of the nice things about the "default" Prometheus setup is that it just gets out of the way. Adding new metrics to client-go is a bit of a pain, IIRC -- last time I looked, it required adding dummy struct implementations, among other things (that might've changed in the interim). I'd prefer to have some setup that was fairly straightforward to understand and update, and didn't require editing multiple files just to add a single metric.
I think in terms of this KEP it's more a nice to have, the main goal is that the metrics names don't keep changing, the plumbing can be improved afterwards I think.
Ack. I'm on board 100% with the rest of the KEP :-)
There was a problem hiding this comment.
Added:
Within the scope of this KEP, we want to explore other ways, however, it is not blocking for its success, as the primary goal is to make the metrics exposed themselves more consistent and stable.
Making it a non-blocker, and more exploratory territory. All I'm saying is, the apiserver should not have etcd server metrics, because of poor choice of globals, and more generally metrics shouldn't suffer because of poor architecture. A production system is hard work, and making it observable is part of that.
|
REMINDER: KEPs are moving to k/enhancements on November 30. Please attempt to merge this KEP before then to signal consensus. Any questions regarding this move should be directed to that thread and not asked on GitHub. |
|
|
||
| Change the container metrics exposed through cAdvisor (which is compiled into the Kubelet) to [use consistent labeling according to the instrumentation guidelines](https://github.com/kubernetes/kubernetes/pull/69099). Concretely what that means is changing all the occurrences of the labels: | ||
| `pod_name` to `pod` | ||
| `container_name` to `container` |
There was a problem hiding this comment.
How about existing metrics scrapes which rely on the current labeling? Do we provide some deprecation period where old metric labels would still be available?
|
|
||
| Risks include users upgrading Kubernetes, but not updating their usage of Kubernetes exposed metrics in alerting and dashboarding potentially causing incidents to go unnoticed. | ||
|
|
||
| To prevent this, we will implement recording rules for Prometheus that allow best effort backward compatibility as well as update uses of breaking metric usages in the [Kubernetes monitoring mixin](https://github.com/kubernetes-monitoring/kubernetes-mixin), a widely used collection of Prometheus alerts and Grafana dashboards for Kubernetes. |
There was a problem hiding this comment.
ok, this answered my question from above :-D
|
I am 👍 on this proposal :-) |
| #### Consistent labeling | ||
|
|
||
| Change the container metrics exposed through cAdvisor (which is compiled into the Kubelet) to [use consistent labeling according to the instrumentation guidelines](https://github.com/kubernetes/kubernetes/pull/69099). Concretely what that means is changing all the occurrences of the labels: | ||
| `pod_name` to `pod` |
There was a problem hiding this comment.
This sounds fine to me. Would we just have a release where both labels are present?
There was a problem hiding this comment.
As proposed I think I would prefer providing Prometheus recording rules that allow for backward compatibility, this is what’s been done in the Prometheus ecosystem. But I have no strong feelings about this, I’m also happy with a transition release having both.
|
modulo my mini-rant above about plumbing through metrics handles, I'm 👍 on this |
brancz
force-pushed
the
metric-overhaul
branch
from
0bee38d
to
dd9b7a1
Compare
k8s-ci-robot
added
the
cncf-cla: yes
|
Changed the wording of the "removal" of the global metrics registry pattern to an exploratory topic of enhancing metrics, so we can explore it, but consistency and stability of exposed metrics is the priority. If we can it would be nice if we can get this merged by Nov 30. @DirectXMan12 @piosz @coffeepac @dashpole 🙂 |
|
/approve from me (not that I can approve it officially, but the content looks good from my side) |
|
/assign @jbeda |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12, jdumars The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/lgtm |
k8s-ci-robot
added
the
lgtm
keps/sig-instrumentation: Add metrics overhaul KEP
This pull requests adds a living KEP that has been brewing within sig-instrumentation for the past few weeks. It outlines an overhaul of metrics defined in kubernetes/kubernetes, to be conform with Prometheus best practices as well as the Kubernetes instrumentation guide. Read on to discover more details 🙂 .
Catch phrase: A road to compatibility guarantees of Kubernetes exposed metrics.
/cc @piosz @DirectXMan12 @coffeepac @metalmatze @s-urbaniak @mxinden