Handle NPE when "HELP" option is not provided

[dlazerka]

Hello, I'm trying to create a metric without any HELP set (it is allowed per exposition format spec).
However, client_java prevents me of doing that, see https://github.com/prometheus/client_java/blob/master/simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java#L165

If not calling .help() it will fail with NPE, if provided with empty string, it will throw IllegalStateException.

I think it shouldn't throw anything if .help() wasn't called.

[brian-brazil]

Client libraries can require that HELP is set, which is what we do.

[lhenk89]

I'm interested in trying this as my first open source contribution - @brian-brazil please let me know if this should be implemented or not

[brian-brazil]

The present behaviour is correct, help is required.

[jsfuchs]

Having run into this one a few times now, I'm of mixed feelings on the matter.

Yes, help is required. However, it doesn't make a lot of sense to me for a metrics library to crash the program because you didn't specify the help string. Metrics calls are 99% of the time side effects to what your program does.

Would it be ok to provide a default help string (maybe just parrot the metric name) and log a big warning that you should set the help string yourself?

[brian-brazil]

Setting help happens at program startup, not as a side-effect of general runtime. Having a default would defeat the purpose of requiring the user to specify one.

[dlazerka]

Hi guys, sorry, forgot about this issue.

Side note: NPE is always a programming error, never an expected behavior. If you want to handle an expected behavior by raising an exception -- that exception should not be NullPointer.

Re. happening on program startup -- I wouldn't say that's always really so. Some systems can decide to instantiate some metrics clients in response to some other events, especially complex projects. So it may fail in runtime after responding success to health-checks. Of course, complex systems must have automated tests to prevent that. It would be just nicer to do that at compile time.

As for my personal use-case, I don't mind any way, it's just a small script for personal use that just prints the metrics to STDOUT after finishing. There's absolutely zero value for me in the HELP string, so I cleaned it from the output. It was just a little annoying having to specify it in the code only to clean it later. But I don't mind any way, I wrote a small wrapper around it that provides . as help string.

[waisbrot]

This seems to me like using a run-time exception (even if you think it should mostly happen near start-time) for something that would ideally be caught at compile time. NPE/InvalidState/etc are necessary because we can't know what the state will be until we're running.
But in this case, I don't know why you would ever be in a situation where you don't know until run-time whether someone will define the (required) help string. So, it's a shame to be stuck using run-time checks for something that shouldn't need run-time knowledge.

What would you think of something like this:

  static final Summary requestLatency = Summary
    .build("requests_latency_seconds", "Request latency in seconds.")
    .quantile(0.5, 0.05)
    .quantile(0.9, 0.01)
    .register();

?

This changes the API to put the required arguments as part of the Builder. This would make it a compile-time error to create an invalid metric, instead of a run-time error.

[brian-brazil]

That'd require breaking all existing users to work, which I don't think we should do.

[waisbrot]

I was presuming you'd release as a new version and that you're using Semantic Versioning, so the API doesn't claim to be stable yet.

Anyway, I feel like "moves programming errors from run-time to compile-time" is a substantial improvement (whether you take my suggestion or follow some other path), and worth the effort of a breaking change.

[brian-brazil]

This library is widely used, I'm not going to break users without a very good reason.