Skip to content

Make registry optional for MultiProcessCollector#249

Closed
marksteward wants to merge 1 commit intoprometheus:masterfrom
marksteward:master
Closed

Make registry optional for MultiProcessCollector#249
marksteward wants to merge 1 commit intoprometheus:masterfrom
marksteward:master

Conversation

@marksteward
Copy link

@brian-brazil this is a trivial first PR to make it possible to make it easier to add more collectors during a multiproc scrape. This is so the pattern eventually can be:

    registry = CollectorRegistry()
    registry.register(MultiProcessCollector())
    registry.register(PlatformCollector())
    # add your own CustomCollectors here

instead of having to explicitly pass None to each Collector.

I would also have included the change for PlatformCollector, but I'm concerned that might break existing code. Should it be accompanied by deprecation warnings, or is it not worth doing?

@brian-brazil
Copy link
Contributor

I would also have included the change for PlatformCollector, but I'm concerned that might break existing code.

What is there to change?

@brian-brazil
Copy link
Contributor

It's usually expected that you will pass the registry to a collector, rather than having to register it yourself. I'm not sure what you are proposing is actually saving code.

@marksteward
Copy link
Author

marksteward commented Feb 21, 2018

What is there to change?

To make PlatformCollector default to not registering itself with the default registry, except for the default instance. So:

class PlatformCollector(object):
...
    def __init__(self, registry=None, platform=None):
...
PLATFORM_COLLECTOR = PlatformCollector(core.REGISTRY)

This is more explicit and means someone creating their own PlatformCollector instance doesn't end up with one already registered to the default registry by accident.

@marksteward
Copy link
Author

marksteward commented Feb 21, 2018

It's usually expected that you will pass the registry to a collector, rather than having to register it yourself. I'm not sure what you are proposing is actually saving code.

This wasn't clear. The README has:

REGISTRY.register(CustomCollector())

but then:

    multiprocess.MultiProcessCollector(registry)

There's no mention of explicitly passing None to avoid the automatic registration.

When I first looked at this, I assumed MultiProcessCollector was doing some sort of quirky transformation on the registry, rather than just registering itself.

My hope is to make multiproc more self-explanatory and fit better with patterns used in Django, Flask, etc.

I'm happy to change the README instead if it should go the other way.

@brian-brazil
Copy link
Contributor

So collectors are meant to auto-register with the default registry by default. The MultiProcessCollector in unusual in that you never use it with the default registry, and use a custom registry instead.

@marksteward
Copy link
Author

I'll raise a separate PR to improve the README.

@marksteward marksteward closed this Mar 5, 2018
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.

2 participants