services: Fix metric from not publishing

[christarazi]

• metrics, service: Rename metric for clarity
• services: Fix metric from not publishing

Fix missing metric "cilium_services_events_total"

Fixes: #26511

[chancez]

Odd. I'm reading the docs on WithLabelValues to figure out the cause of the behavior described in your commit message. It refers to https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#CounterVec.GetMetricWithLabelValues which says:

Keeping the Counter for later use is possible (and should be considered if performance is critical), but keep in mind that Reset, DeleteLabelValues and Delete can be used to delete the Counter from the CounterVec. In that case, the Counter will still exist, but it will not be exported anymore, even if a Counter with the same label values is created later.

Do we do anything like call Reset or Delete*?

[ysksuzuki]

This code is called before NewLegacyMetrics initializes ServicesCount, and calls NoOpCounterVec.WithLabelValues (returns NoOpGauge). That's the reason services_events_total is missing. Is my understanding correct?

[christarazi]

Yes, I think you're right. I opened a small thread about this in #development and that's what Andre suspected as well. 👍

[christarazi]

cc @chancez

[chancez]

Gotcha. How can we prevent this from reoccurring?

[christarazi]

I'm not too sure to be honest. We have https://github.com/cilium/customvet where we can write our own linting checkers. However, this is the only case that I've found in the code base. Moving forward there is an effort to move metrics into their modules (see #25651) which should help, but techincally not a guarantee.

[christarazi]

That seams quite reasonable to me. I guess I didn't question the structure, but now that I think about it a bit, they're likely defined this way (NoOp) because of the problem of having global metrics defined in one place rather than per module. Due to them being exposed as global vars, they can be accessed at any time without any guarantee that they've been initialized, so NoOp wrapper was created to prevent panics / crashes in case that happened. That's my guess.

cc @aanm (for adding NoOp) and @dylandreimerink (for whether we still need the idea of "NoOp" metrics going forward)

[dylandreimerink]

The option is to get rid of these NoOp metrics potentially. The approach we're using is a bit odd from my perspective but I haven't really dug into why we do it this way

The NoOp metrics exist as a stopgap, currently a lot of the code base still uses global variables to get metrics which is subject to ordering.

they're likely defined this way (NoOp) because of the problem of having global metrics defined in one place rather than per module. Due to them being exposed as global vars, they can be accessed at any time without any guarantee that they've been initialized, so NoOp wrapper was created to prevent panics / crashes in case that happened.

Yes, that is indeed correct.

Ideally, all metrics are provided to components via parameters during construction, but this conversion takes some time. We can remove NoOps once all global metric variables are gone.

[chancez]

There's an intermediate step I mentioned that wasn't addressed: You can have globals, without NoOp metrics, but not call registry.Register(yourMetric). This avoids ordering issues and nil globals.

[christarazi]

Btw, I've updated the second commit's description to describe the global variable vs initialization ordering as the root cause of the bug.

[dylandreimerink]

There's an intermediate step I mentioned that wasn't addressed: You can have globals, without NoOp metrics, but not call registry.Register(yourMetric). This avoids ordering issues and nil globals.

Yes, you could init the global var and then upon creating the registry determine if you want to add that metric to the registry or not.

I am not sure why NoOps were chosen over that approach, it is not something invented for the modular metrics, it was already the way things worked. The main thing that changed was init ordering.

I would argue that removing all global variables in favor of dependency injection ASAP is a better use of cycles then refactoring the NoOps away first.

[ysksuzuki]

The changes look good to me

[christarazi]

@chancez Is there anything that you think is blocking merging this PR?

[christarazi]

/test