Rename labelvalues to _labelvalues to make clear that it's internal

[bm371613]

Metrics with labelvalues are not collected. In the example below, I would expect all 3 counters to be collected, but the one with labelvalues is not.

from prometheus_client import REGISTRY, Counter

c1 = Counter("c1", "c1")
c2 = Counter("c2", "c2", labelnames=["l1"])
c3 = Counter("c3", "c3", labelnames=["l1"], labelvalues=["v1"])

c1.inc()
c2.labels("v1").inc()
c3.inc()

for item in REGISTRY.collect():
    print(item)  # prints c1 and c2, but not c3

[brian-brazil]

That should be producing an exception, that's invalid usage.

[bm371613]

Where should I learn about the intended usage? sob., 13.04.2019, 11:44 użytkownik Brian Brazil <notifications@github.com> napisał:

…

That should be producing an exception, that's invalid usage. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#398 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC8NdtJxKO88W88S99VSe-GTqMv2TYkFks5vgacagaJpZM4ct8rw> .

[brian-brazil]

https://github.com/prometheus/client_python#labels

[bm371613]

It does not mention labelvalues. If labelvalues are not for passing values for labels and my example is invalid usage, the issue should be that labelvalues are not intuitive and not documented. sob., 13.04.2019, 12:35 użytkownik Brian Brazil <notifications@github.com> napisał:

…

https://github.com/prometheus/client_python#labels — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#398 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC8Ndpazs9s3u--8jWNSBzLNbz-EAWseks5vgbMPgaJpZM4ct8rw> .

[brian-brazil]

Non-existent features tend not to be documented.

[bm371613]

I meant the labelvalues argument. It is an argument of a constructor of a public class that is not documented. It would be fine if there was only one reasonable behavior given its name and that was how it worked, but this is not the case. I have an answer now, but I won't close the issue - you may want to keep it open as a reminder that the labelvalues parameter could use a docstring. Otherwise close it. sob., 13.04.2019, 14:54 użytkownik Brian Brazil <notifications@github.com> napisał:

…

Non-existent features tend not to be documented. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#398 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC8NdjqoHroupTSfqWuI-fTRsxNVtwmOks5vgdOEgaJpZM4ct8rw> .

[dash-rai]

It is an argument of a constructor of a public class that is not documented.

I agree with @bm371613 here. It is easily assumed that labelvalues is meant for accepting default values for labels, and there is no indication that this is wrong -- like @brian-brazil said, it doesn't raise an exception.

I'm not sure what the intended use of labelvalues is, but it would be a useful feature to have the ability to set "default" labels or a base set of labels for all child metrics. Support for chaining calls to .labels() would probably be required too.

[ewtoombs]

labelvalues is for internal use only. It is used in the MetricWrapperBase.labels() method to create a new MetricWrapperBase with labelvalues set. Since the metric was already registered, there's an if statement in MetricWrapperBase's constructor that prevents registration of the metric if labelvalues is set.

So, that's why you're seeing the behaviour that you are. The user was never supposed to use the labelvalues argument of MetricWrapperBase in the first place.

This is terrible and it should be fixed! One way to do it is add an internal argument, _register=True, to MetricWrapperBase. Then, MetricWrapperBase.labels() initiates the child MetricWrapperBase with _register=False. Then, labelvalues will have the obvious and assumed behaviour.

Incidentally, if this is what you want, for now, initialise c3 like this instead, as a workaround:

c3 = Counter('c3', 'c3', labelnames=['l1']).labels('v1')

[jonerer]

IMO the current situation is quite confusing and quite time consuming. What do you feel about adding a couple of lines in the documentation to warn about this argument? Or perhaps just a runtime warning for users?

[brian-brazil]

I'm not sure how changing the user docs will help if users are ignoring the docs and instead digging into the source and using undocumented internal APIs.

[jonerer]

In this case it's not clear that it's an internal API. Labelvalues is a kwarg on a public constructor, and it looks like it fills a need that the end user often feels (setting a default value for a Gauge's labels). If it's only meant to be internal then yeah maybe you're right, the best place to make hanger might not be in the documentation.

Renaming it by prepending a "_" is a common pythonic way of marking something as internal. Maybe that?

[brian-brazil]

That sounds reasonable.

I'll note that a default value for a label doesn't really make sense, you need to have code to provide the actual label value one way or the other.

[belm0]

Having a static set of known label sets is the common (actually only) case for us. We want to register the known label sets at metric declaration time, with the least amount of fuss.

I've come up with this wrapper, but really I'd like to see a labelsets option in the metric constructors.

def init_labelsets(metric, labelsets: Iterable[Dict]):
    """Initialize known label sets of a Prometheus metric

    For a metric without labels, the client exports an initial value as
    expected (0 for Counter, etc.).  But when a metric has labels, the client
    cannot know the label values and combinations in advance.  This function
    can register known label sets so that the initial value is exported.

    Suggested use is as a wrapper of the metric declaration:

    >>> metric = init_labelsets(
    ...     prometheus_client.Counter('foo', 'count of foo', ['type']),
    ...     (dict(type=v) for v in ('A', 'B', 'C'))
    ... )
    >>> list(metric._samples())
    [('_total', {'type': 'A'}, 0.0),
     ('_total', {'type': 'B'}, 0.0),
     ('_total', {'type': 'C'}, 0.0)]

    Example with a label combination:

    >>> metric2 = init_labelsets(
    ...     prometheus_client.Counter('bar', 'count of bar', ['label1', 'label2']),
    ...     (dict(label1=v1, label2=v2) for v1 in ('A', 'B') for v2 in ('0', '1'))
    ... )
    >>> list(metric2._samples())
    [('_total', {'label1': 'A', 'label2': '0'}, 0.0),
     ('_total', {'label1': 'A', 'label2': '1'}, 0.0),
     ('_total', {'label1': 'B', 'label2': '0'}, 0.0),
     ('_total', {'label1': 'B', 'label2': '1'}, 0.0)]
    """
    for labelset in labelsets:
        metric.labels(**labelset)
    return metric

[brian-brazil]

@belm0 That is unrelated to this issue.

[belm0]

It's related because the people attempting to use labelvalues are wanting precisely this functionality, as the OP concedes.

[brian-brazil]

No, they're not looking for initialisation of childs. Please don't confuse the issue.