Add PrometheusMetricsFactory

[eundoosong]

As discussed in PR 129 with @yurishkuro, PrometheusMetricsFactory is ready for review.

Signed-off-by: Eundoo Song eundoo.song@gmail.com

[coveralls]

Coverage increased (+0.1%) to 93.883% when pulling 4814b3e on eundoosong:add_prometheus_metrics_factory into 58173eb on jaegertracing:master.

[eundoosong]

@yurishkuro
If you see the coverage result, it is decreased because pytest-cov works as if there is no unit-testing for prometheus_metrics.py
I don’t want to run unit-testing(pytest) and coverage(pytest-cov) on PrometheusMetricsFactory if prometheus_client is not installed.
So skipif decorator provided by pytest is added in test_prometheus_metrics.py for unit-testing skipping, which works good.

@pytest.mark.skipif('prometheus_client' not in sys.modules,  
                                     reason="requires prometheus_client")
class TestPrometheusMetricsFactory:
    def test_prometheus_metrics_counter(self):
        ....

But I can’t find any similar way(conditional skip) to do the same in pytest-cov.
Options only provided by pytest-cov is adding "exclude list" into .coveragerc’s omit for file OR "# pragma: no cover" for code in a file.
They are fixed list, not like conditional thing, and no co-working with pytest

I have no idea how get the coverage passed in ci-testing.
Would you please give better suggestion on this?

Alternatively, I prepared update-coveragerc.sh for adding a file path in “omit” when a library is not installed. If it looks good, I can merge this.
https://github.com/eundoosong/jaeger-client-python/blob/prometheus_poc/scripts/update-coveragerc.sh

[yurishkuro]

overall looks great. I just have a couple organizational questions:

1. what benefits do you see in placing the new class in a subdirectory instead of, say, jaeger_client/prometheus.py? E.g.

from jaeger_client.prometheus import PrometheusMetricsFactory

2. our main readme is not very large, I would add the Prometheus instructions there instead of hiding in a subdirectory (where it's much harder for people to find).

[yurishkuro]

I don’t want to run unit-testing(pytest) and coverage(pytest-cov) on PrometheusMetricsFactory if prometheus_client is not installed.

Why not always run them? prometheus_client can be declared as dev dependency, as we do with other modules like opentracing instrumentation or tchannel (https://github.com/jaegertracing/jaeger-client-python/blob/master/setup.py#L64)

[eundoosong]

@yurishkuro

1. prometheus_client as dev dependency
   Agree, it is more simple and clear. Thanks.

2. What benefits do you see in placing the new class in a subdirectory instead of, say, jaeger_client/prometheus.py?
   I think prometheus_metrics is somewhat not part of jaeger client, it's more like an optional plugin for metrics purpose.
   And probably there might be other metrics coming in the future too. So, I moved it to the subdirectory.
   Please tell me your thoughts,

3. Add Prometheus instructions in main readme.
   Agree.

[yurishkuro]

LGTM

I think prometheus_metrics is somewhat not part of jaeger client, it's more like an optional plugin for metrics purpose. And probably there might be other metrics coming in the future too. So, I moved it to the subdirectory.

fair enough. I'd just ask you to DRY the naming, i.e

# instead of this
from jaeger_client.metrics_factory.prometheus_metrics import PrometheusMetricsFactory

# support this
from jaeger_client.metrics.prometheus import PrometheusMetricsFactory

[yurishkuro]

I think it's better to move this to L83 as level-4 header, i.e. as a subsection of Configuration/Production.

[eundoosong]

moved

[yurishkuro]

maybe namespace='your-app-name' for consistency?

[eundoosong]

add namespace='your-app-name'

[eundoosong]

instead of this
from jaeger_client.metrics_factory.prometheus_metrics import PrometheusMetricsFactory

support this
from jaeger_client.metrics.prometheus import PrometheusMetricsFactory

Earlier, I named metrics as subdirectory, but naming conflict error came like below since metrics.py and metrics subdirectory existed in the same location.
So I simply renamed to metrics_factory to resolve the conflict.
If you have better idea (or better naming), please tell me.

$ python example_prometheus.py
Traceback (most recent call last):
  File "example_prometheus.py", line 3, in <module>
    from jaeger_client import Config
  File "/Users/eundusong/dev/opensource/jaeger/dev/jaeger-client-python/jaeger_client/__init__.py", line 26, in <module>
    from .tracer import Tracer  # noqa
  File "/Users/eundusong/dev/opensource/jaeger/dev/jaeger-client-python/jaeger_client/tracer.py", line 33, in <module>
    from .metrics import Metrics, LegacyMetricsFactory
ImportError: cannot import name Metrics

[yurishkuro]

Earlier, I named metrics as subdirectory, but naming conflict error came like below since metrics.py ...

I see. I just put a PR #143 that moves metrics.py into a subdirectory. It looks like it's backwards compatible. Let's merge that first, then you can rebase and move your code to jaeger_client/metrics/prometheus.py

[eundoosong]

@yurishkuro
moved to "metrics/"
Ready to be merged, or is there anything else missing?

[yurishkuro]

looks great. Thanks!