NullPointerException in Java API

[njbartlett]

In the Java API, the following code works:

private final Gauge fillLevelGauge = Gauge.build()
        .name("fill_level")
        .help("Fill level")
        .register();

    // ...

    fillLevelGauge.set(1d);

... but the following code crashes with a NullPointerException:

private final Gauge fillLevelGauge = Gauge.build()
        .name("fill_level")
        .labelNames("mylabel")
        .help("Fill level")
        .register();

    // ...

    fillLevelGauge.set(1d);

I found the following issue which states "for a labelled metric you must specify the labels". However I cannot believe that you actually intend for the program to crash when a programmer makes this mistake? And not even with any explanation of the reason for the crash?

At the very least I believe this should be an IllegalArgumentException with a message attached, e.g. "for a labelled metric you must specify the labels".

NB PLEASE don't just close this with your common response to ask questions in the user mailing list. This is not a usage question, I am reporting an actual bug in the software.

[brian-brazil]

You need to provide a string as documented, nulls are not strings.

[njbartlett]

My code does not pass any nulls.

[brian-brazil]

Ah right, I misread.

This is a programming error and throwing an exception is correct and there is no way to recover from it, that method is only for metrics with no labels. Had you called labels() as is documented but with the wrong number of arguments you would have gotten an IllegalArgumentException.

[njbartlett]

The problem is that a naked NullPointerException gives the programmer no information on what he/she has done wrong. This happens because in Gauge.java (and other SimpleCollector subclasses) you dereference the noLabelsChild field without checking whether it has been initialized (in methods inc, dec, set, etc). This could be easily addressed by checking the field and raising an IllegalArgumentException at this point.

I would be willing to create a PR for this if there is a reasonable chance of it being accepted.

[brian-brazil]

I'll accept it if it causes no performance degradation, this is a hot path.

[njbartlett]

My fix will be roughly as follows:

1. Make SimpleCollector#noLabelsChild private.
2. Write a protected method on SimpleCollector called getNoLabelsChild, in which the null check and throw new IllegalArgumentException occurs.
3. Make SimpleCollector subclasses access noLabelsChild via that method.

The getter method will likely be rapidly inlined by Hotspot so I do not believe any noticeable performance degradation would occur.

[jtktam]

@brian-brazil for Histogram there is no "labels()" method.. what is the correct documented way to add labels to a histogram?

thanks

[brian-brazil]

@jtktam Please ask usage questions on the prometheus-users mailing list rather than on an unrelated Github issue.

[jtktam]

How is it unrelated if you are the one suggesting to use labels method which does not exist? I have opened a new issue specifically with histogram and the same issue with nullpointexception

[ievgen-kapinos]

For those who tired to investigate problem root. This feature allows to add few series in one gauge.

val collectorRegistry = CollectorRegistry()
val syncGauge = Gauge.build()
      .subsystem("app")
      .name("synchronizer")
      .labelNames("state")
      .help("Consumer/exporter synchronizer state")
      .register(collectorRegistry)

val gaugeConsumedIndex = syncGauge.labels("consumed_index")
val gaugeExportedIndex = syncGauge.labels("exported_index")
val gaugePendingExportedIndex = syncGauge.labels("pending_exported_index")

you will get

# HELP app_synchronizer Consumer/exporter synchronizer state
# TYPE app_synchronizer gauge
app_synchronizer{state="exported_index",} 0.0
app_synchronizer{state="pending_exported_index",} 0.0
app_synchronizer{state="consumed_index",} 0.0