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

_MultiProcessValue should resolve pid dynamically #173

Closed
wants to merge 1 commit into from
Closed

_MultiProcessValue should resolve pid dynamically #173

wants to merge 1 commit into from

Conversation

dkruchinin
Copy link

I maintain a prometheus metrics collection library for Sanic web service and I made an attempt to add multiprocessing support to it which didn't go well. The problem was that _MultiProcessValue resolves pid at the time the module is imported, which is fine for gunicorn-based apps but it just doesn't work when a python app forks after the module is imported, like Sanic does.

I wrote a super simple service to demonstrate the problem:

import os
import sys
import time
from sanic import Sanic
from sanic.response import text, raw
from prometheus_client import (
    generate_latest,
    CollectorRegistry,
    CONTENT_TYPE_LATEST,
    Counter,
    multiprocess
)

NREQS = None
app = Sanic()


@app.route('/test')
async def test(request):
    NREQS.inc()
    return text("[{}] ...\n".format(os.getpid()))


@app.route('/metrics')
async def metrics(request):
    registry = CollectorRegistry()
    multiprocess.MultiProcessCollector(registry)
    data = generate_latest(registry)
    return raw(data, content_type=CONTENT_TYPE_LATEST)


@app.listener('before_server_start')
def before_start(app, loop):
    global NREQS
    NREQS = Counter("num_requests", "Example counter")


@app.listener('after_server_stop')
def after_stop(app, loop):
    multiprocess.mark_process_dead(os.getpid())


if __name__ == '__main__':
    if len(sys.argv) != 2:
        print("USAGE: {} <port>".format(sys.argv[0]))
        sys.exit(1)

    app.run(port=int(sys.argv[1]), workers=4)

If you run this code without the patch, that's what you get:

% mkdir metrics
% env prometheus_multiproc_dir=metrics python test_sanic.py 8000 &
% for i in `seq 100`; do curl "http://localhost:8000/test"; done
...
[45495] ...
[45494] ...
...
% curl http://localhost:8000/metrics
# HELP num_requests Multiprocess metric
# TYPE num_requests counter
num_requests 70.0 # expected to be 100 though 
% ls metrics
counter_45490.db

With the patch applied:

% rm -rf metrics/*
% env prometheus_multiproc_dir=metrics python test_sanic.py 8000 &
% for i in `seq 100`; do curl "http://localhost:8000/test"; done
...
[45781] ...
[45780] ...
[45778] ...
...
...
% curl http://localhost:8000/metrics
# HELP num_requests Multiprocess metric
# TYPE num_requests counter
num_requests 100.0
% ls metrics
counter_45778.db counter_45779.db counter_45780.db counter_45781.db

@brian-brazil
Copy link
Contributor

Thanks for the PR. It's a bit more complicated than that, see #169

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