No input validation on MetricFamily classes resulting in confusing errors

[wolph]

Using the code from the readme with the small alteration of an incorrect value, an export results in a useless stacktrace.

The code used:

from prometheus_client.core import GaugeMetricFamily, CounterMetricFamily, REGISTRY

class CustomCollector(object):
    def collect(self):
        yield GaugeMetricFamily('my_gauge', 'Help text', value=7)
        c = CounterMetricFamily('my_counter_total', 'Help text', labels=['foo'])
        c.add_metric(['bar'], None)
        c.add_metric(['baz'], 'baz')
        yield c

REGISTRY.register(CustomCollector())

The resulting trace:

Exception happened during processing of request from ('127.0.0.1', 12345)
Traceback (most recent call last):
  File "/usr/lib/python3.6/socketserver.py", line 651, in process_request_thread
    self.finish_request(request, client_address)
  File "/usr/lib/python3.6/socketserver.py", line 361, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/usr/lib/python3.6/socketserver.py", line 721, in __init__
    self.handle()
  File "/usr/lib/python3.6/http/server.py", line 418, in handle
    self.handle_one_request()
  File "/usr/lib/python3.6/http/server.py", line 406, in handle_one_request
    method()
  File "prometheus_client/exposition.py", line 146, in do_GET
    output = encoder(registry)
  File "prometheus_client/exposition.py", line 119, in generate_latest
    output.append(sample_line(s))
  File "/opt/lg5commander/.venv/lib/python3.6/site-packages/prometheus_client/exposition.py", line 86, in sample_line
    s.name, labelstr, floatToGoString(s.value), timestamp)
  File "test.py", line 8, in floatToGoString
    d = float(d)
TypeError: float() argument must be a string or a number, not 'NoneType'

I am not too familiar with the code but I wonder if it would somehow be possible to merge the metrics_core.py code with metrics.py. Or is there a reason that, for example, a Gauge does not use the GaugeMetricFamily internally (or vice versa)?

[brian-brazil]

If you're using the MetricFamilys it's presumed you know what you're doing, and the stack trace is correct as those can't be converted to floats.

[wolph]

I understand the stack trace is correct, but why would there be no input validation on the add_metric? Is there a situation where other value types would be correct?

In my case the value came from a mongo database which had an incomplete record resulting in None. Tracking it down required me to monkeypatch the code, because this error only occurs on runtime with no useful information about the cause.

I would propose either some validation on the add_metric or additional output when raising errors from sample_line so these runtime only errors can be tracked down without firing up a debugger.

[brian-brazil]

Is there a situation where other value types would be correct?

You might have a object that looks like a float.

I would propose either some validation on the add_metric

That'd be doing double the validation.

additional output when raising errors from sample_line

Do you want to send a PR?

[wolph]

Is there a situation where other value types would be correct?

You might have a object that looks like a float.

But if you're going to cast to a float in the end anyhow, would it be a good solution to do the casting sooner? That would immediately solve the issue and wouldn't require any double work.

additional output when raising errors from sample_line

Do you want to send a PR?

That depends on the amount of work required, but if it's not too invasive, sure :)

I've noticed you're not using the logging module anywhere, I think that could be a good solution for adding additional debug information. I expect that there are several other similar error cases that could benefit from this debug information as well (for example, with recursive imports it can be hard to track down the cause of duplicate measurement names).

[brian-brazil]

But if you're going to cast to a float in the end anyhow, would it be a good solution to do the casting sooner?

That may be a bit of a layering violation, different exposition formats use different types.

[wolph]

Ok, so should I go for the logging solution?

Adding some logging and documentation on how to enable/disable the logging within your application? Generally I do something along these lines to make it easy to enable/disable logging within a module:

import logging


logger = logging.getLogger(__name__)


class SomeClass:
    def __init__(self, ...):
        self.logger = logger.getChild(self.__class__.__name__)

Would that be acceptable?

[brian-brazil]

I was thinking more having a better exception in cases like this