MetricVec constructors do not initialize the underlying prometheus metrics

[dgrisonnet]

What happened:

When creating a counter with component-base NewCounterVec and calling metric.Describe(ch), an unexpected segfault occurs.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x818f89]

goroutine 1 [running]:
github.com/prometheus/client_golang/prometheus.(*metricVec).Describe(...)
	/tmp/test/vendor/github.com/prometheus/client_golang/prometheus/vec.go:98
main.main()
	/tmp/test/main.go:17 +0xa9

What you expected to happen:

I was expecting the metric to be describable as when I create it with client_golang NewCounterVec.

How to reproduce it (as minimally and precisely as possible):

package main

import (
        "github.com/prometheus/client_golang/prometheus"
        "k8s.io/component-base/metrics"
)

func main() {
        ch := make(chan *prometheus.Desc, 1)
        counter := metrics.NewCounterVec(
                &metrics.CounterOpts{Name: "test", Help: "help"},
                []string{},                                                                                                                                                 
        )   
        counter.Describe(ch)
}

Anything else we need to know?:

This happens when creating all types of Vec; however, non-Vec metrics such as Counter are not affected by this segfault.

This issue seems to be related to the inner Prometheus metric initialization.
In the NewCounter function, setPrometheusCounter is called. Whereas, in NewCounterVec, the initializeMetric function is not called.

The segfault was left unnoticed because the metric initialization is also done during the metric registration. Thus, to reproduce it we need to call the Describe method before registering the metric.

Environment:

• k8s.io/component-base v0.19.2

/sig instrumentation
/cc @serathius

[ehashman]

/assign @brancz

[brancz]

@dgrisonnet do you want to give it a try to fix this?

[dgrisonnet]

I can give it a spin but I might need some more insights.

For the record, I was able to fix my original problem in metrics-server by forcing the instantiation of the metric by calling the Create method before trying to collect it with testutil.CollectAndCompare. AFAIU, this seems to be the intended way to do it considering the lazy instantiation of the metrics. However, I don't think segfaults should occur in such cases.

I have two different ideas on how to fix this:

• always instantiate the underlying collector to be describable and collectible. This would require the MetricVec implementation of client_golang to be exported Export MetricVec (again) prometheus/client_golang#803.
• override all the functions that access the underlying collector and check if the metric is created. There is already some precedence in the codebase: https://github.com/kubernetes/component-base/blob/c985e940ef8ffde448c517479bf260be98ce596b/metrics/counter.go#L139-L144.

wdyt @brancz?

[serathius]

/cc @RainbowMango @logicalhan
as they authored the metrics framework

[RainbowMango]

The reason why we delay instantiation to the registry is we want to check if the metrics are deprecated or not.
If we want to instantiate it before the registry, how do we know the release version which holds in the registry for now?

I wonder why you need to describe a CounterVec before it is registered? Can you explain more about your cases?

[dgrisonnet]

My use case was in metrics-server's unit tests when we moved from the global registry to the dependency injected ones. To test the metrics, we use the testutil.CollectAndCompare function from the metrics framework that is responsible for registering and collecting a given metric. However, this function is using the client_golang registration mechanism instead of the custom one from this framework. The problem is that the client_golang implementation is not initializing the underlying collector during registration. Thus, upon registration by CollectAndCompare, the underlying collector of the lazily instantiated metric isn't initialized which results in a segfault when client_golang tries to describe and gather the metric.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x7e0852]

goroutine 4 [running]:
github.com/prometheus/client_golang/prometheus.(*metricVec).Describe(0x0, 0xc00007a1e0)
        /tmp/test/vendor/github.com/prometheus/client_golang/prometheus/vec.go:98 +0x22
github.com/prometheus/client_golang/prometheus.(*Registry).Register.func1(0x7f1ac805c008, 0xc00018c300, 0xc00007a1e0)
        /tmp/test/vendor/github.com/prometheus/client_golang/prometheus/registry.go:275 +0x3b
created by github.com/prometheus/client_golang/prometheus.(*Registry).Register
        /tmp/test/vendor/github.com/prometheus/client_golang/prometheus/registry.go:274 +0x166

I solved this by forcing the creation of the metric before calling this function. However, I wonder if this framework's wrapper should be responsible for registering the metric properly.

If we want to instantiate it before the registry, how do we know the release version which holds in the registry for now?

I wasn't clear enough, but when I mentioned instantiating the underlying collector before registration I was referring to initializing it to a dummy metric. For example, I tried instantiating it to prometheus.NewCounterVec(prometheus.CounterOpts{}, []string{}) and it gave me the following error instead of the previous segfault:

registering collector failed: descriptor Desc{fqName: "", help: "", constLabels: {}, variableLabels: []} is invalid: "" is not a valid metric name

The goal would be to make the error easier to understand and avoid any segfaults related to the underlying collector not being instantiated.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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.