Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a clear() method to metric objects to remove all labelsets #642

Merged
merged 1 commit into from
Apr 2, 2021
Merged

Add a clear() method to metric objects to remove all labelsets #642

merged 1 commit into from
Apr 2, 2021

Conversation

Veknelyon
Copy link
Contributor

Hello @brian-brazil,

I wanted to test metric values in my application unit tests but I didn't figured out how to do it (I probably missed something). That's why I propose to add this simple reset() method. Do you think it can be useful? Or is there a better way to test than reseting metrics at the beginning of the test?

And thank you for this very useful lib!

Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, thank you for the contribution!

Typically the best way to test metrics is to create a separate registry for each test for the best isolation, or to unregister the metric from a shared registry at the end of the test. Would one of those methods work for your use case?

@Veknelyon
Copy link
Contributor Author

It does not work in my case because my metrics are global variables shared in my code. So changing the registry does not reset values stored in the metric.
Here is a simplification of my code:

from prometheus_client import Counter, CollectorRegistry, REGISTRY
from pytest import fixture

RUN_COUNTER = Counter('connector_run', labelnames=['tenant'])


def my_function():
    RUN_COUNTER.labels('tenantid').inc()
    # do things


@fixture
def metric_registry():
    registry = CollectorRegistry()
    registry.register(RUN_COUNTER)
    return registry


def test1(metric_registry):
    my_function()
    
    # other asserts


def test2(metric_registry):
    my_function()

    run_count = metric_registry.get_sample_value('connector_run_total', labels={'tenant': 'tenantid'})
    assert run_count == 1.0

(the code is not as simple so I need to keep test1 and test2 separated)

When I run this code assert run_count == 1.0 failed because test1 is executed first (and I can't presume test execution order).

My issue come from the fact that this is not the registry which store the metric value but RUN_COUNTER which is global.
With the reset() method, I would be able to write:

@fixture
def metric_registry():
    RUN_COUNTER.reset()
    registry = CollectorRegistry()
    registry.register(RUN_COUNTER)
    return registry

Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense thanks! I left one question/suggestion, but generally I am happy to add a function like this.

@@ -186,6 +186,11 @@ def remove(self, *labelvalues):
with self._lock:
del self._metrics[labelvalues]

def reset(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you think of naming this remove_all instead of reset to match remove? I could imagine reset also meaning something like "reset all values to zero" or something else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, according to https://prometheus.io/docs/instrumenting/writing_clientlibs/#labels, I think it should be called clear().

Copy link
Contributor Author

@Veknelyon Veknelyon Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the documentation say so. I'll rename it clear.

This method can be usefull to test metrics.

Signed-off-by: Antoine Arbouin <[email protected]>
Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@csmarchbanks csmarchbanks changed the title Add a reset() method to metric objects to remove all labelsets Add a clear() method to metric objects to remove all labelsets Apr 2, 2021
@csmarchbanks csmarchbanks merged commit 1a3ba21 into prometheus:master Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants