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.

Sign up for GitHub

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

Create multiprocess files with process uuid #694

Closed
wants to merge 1 commit into from
Closed

Create multiprocess files with process uuid #694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions prometheus_client/mmap_dict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,13 @@ def close(self):
self._m.close()
self._m = None
self._f.close()
try:
# Given that we're using uuid for the process we can safely
# remove the file because is not going to be shared by other process
os.remove(self._f.name)
except OSError:
# In windows if the file is in use raises an error
pass
self._f = None


Expand Down
23 changes: 18 additions & 5 deletions prometheus_client/multiprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@
import os
import warnings

import psutil

from .metrics_core import Metric
from .mmap_dict import MmapedDict
from .samples import Sample
from .utils import floatToGoString
from .utils import floatToGoString, get_process_data

try: # Python3
FileNotFoundError
Expand Down Expand Up @@ -158,7 +160,18 @@ def mark_process_dead(pid, path=None):
"""Do bookkeeping for when one process dies in a multi-process setup."""
if path is None:
path = os.environ.get('PROMETHEUS_MULTIPROC_DIR', os.environ.get('prometheus_multiproc_dir'))
for f in glob.glob(os.path.join(path, 'gauge_livesum_{0}.db'.format(pid))):
os.remove(f)
for f in glob.glob(os.path.join(path, 'gauge_liveall_{0}.db'.format(pid))):
os.remove(f)
try:
process = psutil.Process(pid)
types = ["counter", "histogram", "summary", "gauge_min", "gauge_max", "gauge_livesum", "gauge_liveall"]
for typ in types:
for f in glob.glob(os.path.join(path, '{0}_{1}_{2}.db'.format(typ, pid, process.__hash__()))):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this safe? Wouldn't this reset some counters partially, therefore producing bad data?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for f in glob.glob(os.path.join(path, '{0}_{1}_{2}.db'.format(typ, pid, process.__hash__()))):
for f in glob.glob(os.path.join(path, '{0}_{1}_{2}.db'.format(typ, pid, hash(process)))):

Isn't hash() the pythonic way to get a hash?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only affect files for a destroyed/stopped worker. I can see the point of not being able to scrape the data because we're removing the files so we can use the other approach which is to mark them as defunct and do the cleanup after a number of minutes/hours.

I'm old school haha I'll change to hash instead of hash

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that if you just remove an old file that contained say 100 increments of a counter then when you go to read all of the files during the next scrape your new value will be 100 lower which rate and other counter functions in prometheus interpret as a reset.

Anything with a counter component (histograms, though not gauge histograms, sum and count fields on a summary, etc...), will need to be merged and have those old increments kept around somewhere. Ideally in one file instead of the current behavior of keeping more and more files on disk as PIDs churn.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you for the explanation, but shouldn't be that scenario happening when the server is restarted and all the files from the metrics folder are wiped? I thought Prometheus server, once the scrape is done, takes care of it and doesn't need the data in the target

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with the current code happens when a process is stopped and cleaned up, but the server is not wiped. You then end up in a state where there is a partial reset (counter value is lowered just by the amount of requests handled by the now stopped and cleaned up process) which can cause large issues with rate calculations.

How a partial reset is handled is that if a counter goes from 100 to 80 on cleanup (partial reset) it is assumed by Prometheus that the counter went from 100 to 0 to 80, causing a big spike during that period of time, when actually no requests may have come through.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I knew about the reset stuff but I thought Prometheus doesn't take the disappeared metric as a 0 but just as disappeared and didn't take into account. Not very much into the internals of Prometheus yet.

So, what would be the suggestion to clean up those files safely? We're removing the files if there is a fork when the mmap object is closed but when the process is dead we can only mark them as defunct and then do the clean up later or something like that? That wouldn't fix the problem because in environments with a high process recycling they're going to hit the same issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One idea is to merge the values into a single file, for example #518 (which is unsafe and has not been updated in awhile). It would be necessary to make sure there are not race conditions or other issues with any new approach.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Then this goes beyond my current knowledge of prometheus. I'll close this pR then. Thank you for your time

os.remove(f)
except (psutil.NoSuchProcess, psutil.AccessDenied):
# We couldn't found the dead process, no further action required because with the uuid of the process there is
# no much we can do
pass
except OSError:
# Error deleting some of the files, for example in windows if they're in use they'll raise this error
# Maybe we can rename them as _defunk if needed to do a future cleanup
pass

16 changes: 16 additions & 0 deletions prometheus_client/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import math
import sys

import psutil

INF = float("inf")
MINUS_INF = float("-inf")
Expand All @@ -22,3 +25,16 @@ def floatToGoString(d):
mantissa = '{0}.{1}{2}'.format(s[0], s[1:dot], s[dot + 1:]).rstrip('0.')
return '{0}e+0{1}'.format(mantissa, dot - 1)
return s

def get_process_data(pid):
try:
process = psutil.Process(pid)
return process.__hash__()
except psutil.NoSuchProcess:
print('Calling process {0} is not running'.format(pid), file=sys.stderr)
raise
except psutil.AccessDenied:
print('Not enough permissions to access process {0}'.format(pid), file=sys.stderr)
raise


13 changes: 9 additions & 4 deletions prometheus_client/values.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
from threading import Lock
import warnings


from .mmap_dict import mmap_key, MmapedDict
from .utils import get_process_data


class MutexValue(object):
Expand Down Expand Up @@ -39,7 +41,9 @@ def MultiProcessValue(process_identifier=os.getpid):
"""
files = {}
values = []
pid = {'value': process_identifier()}
pid = process_identifier()
pdata = {'id': pid, 'uuid': get_process_data(pid)}

# Use a single global lock when in multi-processing mode
# as we presume this means there is no threading going on.
# This avoids the need to also have mutexes in __MmapDict.
Expand Down Expand Up @@ -70,7 +74,7 @@ def __reset(self):
if file_prefix not in files:
filename = os.path.join(
os.environ.get('PROMETHEUS_MULTIPROC_DIR'),
'{0}_{1}.db'.format(file_prefix, pid['value']))
'{0}_{1}_{2}.db'.format(file_prefix, pdata['id'], pdata['uuid']))

files[file_prefix] = MmapedDict(filename)
self._file = files[file_prefix]
Expand All @@ -79,8 +83,9 @@ def __reset(self):

def __check_for_pid_change(self):
actual_pid = process_identifier()
if pid['value'] != actual_pid:
pid['value'] = actual_pid
if pid['id'] != actual_pid:
pid['id'] = actual_pid
pid['uuid'] = get_process_data(actual_pid)
# There has been a fork(), reset all the values.
for f in files.values():
f.close()
Expand Down
3 changes: 3 additions & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
'prometheus_client.openmetrics',
'prometheus_client.twisted',
],
install_requires=[
'psutil'
],
extras_require={
'twisted': ['twisted'],
},
Expand Down