search

prometheus / client_python

Prometheus instrumentation library for Python applications
Apache License 2.0
3.96k stars 798 forks source link

[Question] Is "REGISTRY" meant to be a decorator for "Collectors"? #548

Open migueleliasweb opened 4 years ago

migueleliasweb commented 4 years ago

Browsing through the code I've found a function called write_to_textfile in the exposition module. This function signature looks like this:

def write_to_textfile(path, registry):
    """Write metrics to the given path.
    pass

If you read the implementation of the function, turns out the expectation is not really a registry but instead anything that implements (something like an interface) the .collect() method.

Is that by design?

If so, would it be welcomed the addition of type hints throughout the code? I would be happy to add them and create a PR.

brian-brazil commented 4 years ago

It takes a registry, as the name clearly indicates.

migueleliasweb commented 4 years ago

Thanks @brian-brazil, you couldn't have been more clear. Does that mean adding the type hints is a welcomed addition?

brian-brazil commented 4 years ago

What would that involve?

beregon87 commented 4 years ago

@migueleliasweb that would break compatibility with all python versions < 3.5

brian-brazil commented 4 years ago

That's not on the cards then.

migueleliasweb commented 4 years ago

Hey @brian-brazil , adding the type hints would be just adding the specific type notations to the python files. That's quite handy as it would improve the autocomplete and hints from IDEs.

Some of the examples: https://mypy.readthedocs.io/en/stable/cheat_sheet_py3.html

migueleliasweb commented 4 years ago

@beregon87 That might be true. Which versions are you guys supporting atm?

beregon87 commented 4 years ago

I'm not a maintainer, but looking at setup.py:

        "Programming Language :: Python :: 2.6",
        "Programming Language :: Python :: 2.7",
        "Programming Language :: Python :: 3",
        "Programming Language :: Python :: 3.4",
        "Programming Language :: Python :: 3.5",
        "Programming Language :: Python :: 3.6",
        "Programming Language :: Python :: 3.7",
        "Programming Language :: Python :: 3.8",
brian-brazil commented 4 years ago

That still looks to me like it'd break supported versions.

paravoid commented 3 years ago

See #491 which tracks this issue - so I think this is duplicate. Note that as that task says, one can provide type hints without breaking backwards compatibility, by either using annotations in comments, or preferrably, by using type stubs( .pyi).

takeda commented 3 years ago

@paravoid I made a small PR illustrating that we can add types without breaking any compatibility. It passes all unit tests and adds enough type information so my code passes with running mypy with --strict.

I hope that could help to allow adding types. Here are some benefits, for people wondering if it is worth it: