Create multiprocess files with process uuid #694

ethervoid · 2021-09-13T08:52:38Z

This is a draft open to discussion

In order to be able to clean-up the process files once it's stopped or destroyed, we need to identify those files with a UUID. If not, those files are going to be processed and collected and will keep growing until we restart the server or master process to perform the cleanup of the metrics directory

This change uses the psutil library that gives a hash based on PID+Process creation time in order to differentiate two processes with the same PID but those are different.

I have some questions about the feasibility of this change:

Is the approach ok? Mainly is respectful in case of exception and even though some files could be left behind is not the same situation as we have now. Also, there is an improvement that I can add and is marking those files I couldn't delete as "type_pid_uuid_defunct" so we can do something to directly remove those files
Is it ok to start using an external library? I've seen you're trying to avoid it
Tests are failing because the test process is not running and the unittest.mock library is only from 3.3 it's ok to move all the multiprocess tests to required that version to run?

This change aims to fix #568

// @csmarchbanks

Signed-off-by: Mario de Frutos [email protected]

In order to be able to clean-up the process files once it's stopped or destroyed, we need to identify those files with an UUID. If not, those files are going to be processed and collected and will keep growing until we restart the server or master process to perform the cleanup of the metrics directory This change uses the psutil library that gives a hash based on PID+Process creation time in order to differentiate two processes with the same PID but that are different This change fixes prometheus#568 Signed-off-by: Mario de Frutos <[email protected]>

csmarchbanks

Thank you for the PR. I think the idea behind this (identifying a process based on PID & start time) could work, however, as you mention, one of the goals of this project is to not require external dependencies. I wonder if you could generate a start time (even if approximate) without the psutil library?

roidelapluie · 2021-09-16T07:38:42Z

prometheus_client/multiprocess.py

+        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__()))):


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

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?

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

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.

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

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.

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.

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.

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

roidelapluie · 2021-09-16T07:43:09Z

I'd be very cautious with this pull request. This raises some questions, notably the need for all the try/except.

ethervoid · 2021-09-16T08:05:21Z

@roidelapluie yep, that's why it's a draft and not a proper PR.

I can see the point but we need to be sure the process data is still there because we're relying on the process creation time to differentiate two processes with the same PID in the worst case we should do a cleanup of files not modified in specific time if we lost track of the process hash

ethervoid · 2021-09-16T08:08:27Z

@csmarchbanks yeah we can do it but we need, for performance reasons, to do a native part of the code and has to be multi-environment but that could lead to an increased risk for this PR because it has to deal with the code for the library and also the native C code to get the creation_time for the process

csmarchbanks reviewed Sep 15, 2021

View reviewed changes

roidelapluie reviewed Sep 16, 2021

View reviewed changes

ethervoid closed this Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create multiprocess files with process uuid #694

Create multiprocess files with process uuid #694

ethervoid commented Sep 13, 2021 •

edited

Loading

csmarchbanks left a comment •

edited

Loading

roidelapluie Sep 16, 2021

roidelapluie Sep 16, 2021

ethervoid Sep 16, 2021

csmarchbanks Sep 16, 2021

ethervoid Sep 17, 2021

csmarchbanks Sep 21, 2021

ethervoid Sep 22, 2021

csmarchbanks Sep 22, 2021

ethervoid Sep 23, 2021

roidelapluie commented Sep 16, 2021

ethervoid commented Sep 16, 2021 •

edited

Loading

ethervoid commented Sep 16, 2021 •

edited

Loading

	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)))):

Create multiprocess files with process uuid #694

Create multiprocess files with process uuid #694

Conversation

ethervoid commented Sep 13, 2021 • edited Loading

This is a draft open to discussion

csmarchbanks left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roidelapluie commented Sep 16, 2021

ethervoid commented Sep 16, 2021 • edited Loading

ethervoid commented Sep 16, 2021 • edited Loading

ethervoid commented Sep 13, 2021 •

edited

Loading

csmarchbanks left a comment •

edited

Loading

ethervoid commented Sep 16, 2021 •

edited

Loading

ethervoid commented Sep 16, 2021 •

edited

Loading