.label() Method throws Exceptions

[Tigraine]

Apparently if labels are missing or a null value is supplied to a label the prometheus client will throw a IllegalArgumentException.

Exceptions being thrown on the "hot" path with null values in labels are a problem as they either require me to add try/catch logic around my monitoring code or decorate the whole prometheus client with try/catch blocks.

IMO there are a ton of instances where you might have a label that is not compile-time supplied (I know in most examples it's the HTTP method being passed).
But for a lot of cases it could be something like a AWS Region-ID or a Cluster-ID or a machine name or endpoint name etc..
Not saying that any of these values being null is a good thing, but errors happen sometimes and especially when you are dealing with legacy systems these things are commonplace.
Having code break in very unexpected ways because prometheus complains about it's arguments is really really problematic.

The offending code in the client is here:
https://github.com/prometheus/client_java/blob/master/simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java#L64-L68

Is there any policy on metrics? Because in my book metrics/logging should be totally transparent and never require any additional logic. If I can't call the prometheus client with absolute certainty that it won't break my code under ANY circumstances I'm hesitant to include it as I am opening myself up to another level of complexity (and all of this suddenly needs testing etc).

Imo it's preferable to have missing labels be mis-reported as empty string and the client being 100% runtime safe than having a operation fail because some parameter was not correctly checked.

[brian-brazil]

We require a string, and the problem is that the empty string is a valid value for a label value so then you couldn't distinguish.

[Tigraine]

I know - but having monitoring code break applications is the even worse case imo.
I'd propose something like a strict/lax mode on the builder where you can express intentionally that you want this null-checked.

My concern is that I want my team to easily and without requiring additional testing etc to be able to sprinkle prometheus metrics throughout the applications without having to think about it breaking something or not.

[Tigraine]

I could also see something like this:

 new Counter.Builder()
                .name("queries_failed_total")
                .help("Number of queries that failed")
                .labelNames("method", "org", "reason")
                .nullHandler(Counter.TreatMissingLabelsAsEmptyString)
                .register();

[brian-brazil]

If you want liberal sprinkling, then an extra line of code you have to add kinda goes against that. This is something that'd have to work by default.

[Tigraine]

I'd love to send a PR to address this - but for that I'd need to know what you think about this.
Is having a exception-less count-path something you'd consider?

[brian-brazil]

I'd consider it.

[lackhoa]

Please also use checked exception for common errors such as a missing label, so that production servers don't blow up at runtime when a counter is incremented.

[brian-brazil]

Checked exceptions would add a significant development burden on anyone using this code, as instrumentation should be lightweight and now you'd need to wrap everything in a try/except. Even the most basic of unittesting should catch programming errors such as mismatched labels before code ever hits production, so I don't think checked exceptions would be proportionate - and it'd also be a massive breaking change for everyone.

[lackhoa]

instrumentation should be lightweight

Agreed, everything should be lightweight, but sadly that's not the reality isn't it?

So your solution is to use unittesting. You expect that my code should have a test case for when the value of a particular label is null? And how would people even figure to test for that before their code break?

[brian-brazil]

You said mismatched labels, not null labels - that's something different. If you've mismatched (i.e .the wrong number of ) labels then anything that executes that line of code is sufficient testing wise.

[lackhoa]

Well I said "missing label" but sure, I wasn't clear enough.

The point about null labels still stands, though.

[brian-brazil]

Unfortunately Java doesn't have the notion of non-nullable types, so there's not much that can be done.