Skip to content

Support multiprocessing metrics #179

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

Merged
merged 3 commits into from
Oct 11, 2020

Conversation

Agalin
Copy link
Contributor

@Agalin Agalin commented Oct 9, 2020

Prometheus client does not work well in multiprocessing environments (i.e. basically all WSGI servers including Gunicorn).
It's easy to configure it in multiprocess mode tho.

While easy to enable it's not so easy to test. I've tried to reuse existing metrics tests but due to metrics initialization at module import time I'd need to either:

  • Patch self._value in each global metric (even more than that for histogram).
  • Change those metrics to be instance-specific, initialized during Microservice instance creation.
    Which one you'd like more? I'd definitely prefer second option.

Also existing tests are buggy - there are inter-test dependencies, order matters. It's probably due to those global counters - metrics tests check for output that is generated during execution of other tests, notably response 200 for uri / which is not configured in metrics - and uses completely different service name...

@coveralls
Copy link

coveralls commented Oct 9, 2020

Pull Request Test Coverage Report for Build 947

  • 85 of 87 (97.7%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.07%) to 99.082%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/test_metrics.py 76 78 97.44%
Totals Coverage Status
Change from base Build 943: -0.07%
Covered Lines: 1726
Relevant Lines: 1742

💛 - Coveralls

@avara1986 avara1986 requested a review from alexppg October 9, 2020 19:31
@Agalin Agalin changed the title WIP: Support multiprocessing metrics Support multiprocessing metrics Oct 10, 2020
@Agalin
Copy link
Contributor Author

Agalin commented Oct 10, 2020

Uff. Found a way to test it in a (mostly) clean way. It now fixes metrics leaking into other tests. Making metrics local wouldn't work due to name collision. Also Jaeger config is global anyway so it needs reset through protected members.

BTW would be nice to do something to avoid repeated Jaeger configuration in tests - it unnecessarily spams log output with warnings.

@alexppg
Copy link
Member

alexppg commented Oct 10, 2020

Nice, thanks! It was difficult to make those tests, even that way, so thanks for fixing them.

My only question is, when using the custom registry do we loose the metrics included by default?:

# HELP python_gc_objects_collected_total Objects collected during gc
# TYPE python_gc_objects_collected_total counter
python_gc_objects_collected_total{generation="0"} 2553.0
python_gc_objects_collected_total{generation="1"} 356.0
python_gc_objects_collected_total{generation="2"} 0.0
# HELP python_gc_objects_uncollectable_total Uncollectable object found during GC
# TYPE python_gc_objects_uncollectable_total counter
python_gc_objects_uncollectable_total{generation="0"} 0.0
python_gc_objects_uncollectable_total{generation="1"} 0.0
python_gc_objects_uncollectable_total{generation="2"} 0.0
...

@Agalin

PD: Btw, I'm working on migrating to opentelemetry right now. When done, will supersede this implementation, but since I don't know how long will it take, it's still good.

@Agalin
Copy link
Contributor Author

Agalin commented Oct 11, 2020

Those are not supported but not due to registry - instead it's an explicit limitation of the multiprocess mode.

    Registries can not be used as normal, all instantiated metrics are exported
    Custom collectors do not work (e.g. cpu and memory metrics)
    Info and Enum metrics do not work
    The pushgateway cannot be used
    Gauges cannot use the pid label

I'd assume that python gc metrics are included in memory metrics.

@alexppg
Copy link
Member

alexppg commented Oct 11, 2020

I see. I've never used them, and it seems more important to have the metrics be multi-process, so LGTM.

@avara1986 avara1986 merged commit 03a033a into python-microservices:master Oct 11, 2020
@avara1986 avara1986 added hacktoberfest-accepted Improvement Not a bug but... could be better labels Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Improvement Not a bug but... could be better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants