Should the same metric be passed a different count of parameters?

Two other things to think about:

1. How can we exercise such metrics to ensure that the codepaths are being hit in our test workflows
2. Apparently these calls can cause the entire binary to panic, which seems highly risky to me. I wonder if we can be using alternate APIs that provide stronger guarantees around this.. it's making me a bit concerned in general about how we use metrics and whether we're mitigating the potential risks associated with panicking when metrics are incremented.

cc @cilium/metrics for awareness and consideration of the above topics.

Should the same metric be passed a different count of parameters?

Good catch, probably happened during one of the intermediate iterations, when switching back to using WithLabelValues. @antonipp Could you please take care of a follow-up fix?

Apparently these calls can cause the entire binary to panic, which seems highly risky to me.

Not a metrics person, but to me panic'ing seems fine, because it is basically a bug, and the earlier we catch it during the development cycle, the better (I'm kind of surprised that we haven't already seen it in CI 😕).

Here is the fix: #40817

panicking the cilium-agent due to a bug is not OK.

IMO it is appropriate to distinguish the type of bug. I totally agree with you that the agent should never panic because of e.g., missing validation, not checking error conditions, and things like that. But I personally think it still makes sense in these cases, realistically it seems unrealistic to me wrapping all of these calls with if err != nil { log(err) }, just to end-up catching them in CI in the very same way (but with a way more verbose and hard to read code). To me, it doesn't look much different from MustParseIP("1.2.3.4") (with the string hardcoded obviously, not user-provided), where there could be obviously a typo slipping through, but the likelihood is extremely low (arguably lower, but not totally impossible).

There's generally one place we accept panic, which is as part of config verification on startup.

There are ~370 occurrences of WithLabelValues (g grep WithLabelValues -- ':!vendor' ':!*_test.go' | wc -l), so there are at least two 😁. And a non-negligible number in vendored packages as well, such as controller-runtime. Joking aside, realistically, there will be always panics, otherwise we'd have bug free software (unless we wrap all goroutines with recover, but then we'd have other problems). And I personally struggle at seeing how checking an error that must not occur would help much avoiding that (as you can make a mistake writing the labels, you can forget checking the error).

We did see this in CI, but it was not reliable - most likely the code paths here are only sometimes hit.

Yes, I agree it was not fully reliable, but it still got caught in less then 12 hours, which seems fairly reasonable to me. It should ideally have been caught before merging the PR (either via reviews, and some blame is on me here, or via CI). If it were to be an error log, it would have ended up in exactly the same way.

I agree that doing error checking for all of these is cumbersome. This feels like something that we could statically analyze for. 🤔 I'd much prefer that over extra error checking boilerplate if it solves the same problem.

There are ~370 occurrences ...

This is the reason I got a bit nervous in this thread 😅 at least I had not been thinking about this failure condition. I think we've moved towards unconditionally registering all metrics on startup which should help to reduce the likelihood of hitting this, but metrics are inherently about conditionally incrementing some counters at runtime based on the logic of the program. So if we don't have static analyzers for the incrementing logic then I think we need to be particularly vigilant that the test coverage catches this.

We did catch this relatively quickly, though it was not particularly reliable - this exhibited in various different ways in different workflows, for instance in some cases it was only the "features check" at the end of the check that failed, because it tried to query something in the operator. I'd guess the failure rate <1% of individual test job runs hit it. There's two properties I'd like out of our metrics programming patterns if we could - one, to fail in a consistent way and two, when it fails it produces an understandable error condition. Any ideas we might have to improve these properties would be welcome.

though it was not particularly reliable - this exhibited in various different ways in different workflows, for instance in some cases it was only the "features check" at the end of the check that failed, because it tried to query something in the operator.

Ah, yeah, I guess one aspect is that this metric was incremented in case the reconciliation failed, so we actually needed a failure to trigger that. The other is that the check of operator restarts is currently disabled, as it proved to be flaky in CI (as the operator can restart if it loses the leader election lease). We could probably make that logic a bit smarter to only ignore restarts caused by expected reasons.

I see. I didn't see a related issue so I filed #40858 to follow up on that case in particular.

I ended up writing a linter and it looks like we've got other code that is dynamically constructing the set of labels to pass to the metrics counter functions which seems like it opens up even further to the idea that cilium-agent could randomly crash due to runtime state. I'll follow up separately regarding that.

Related PR: #40863