-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add socket backlog metric #2407
base: master
Are you sure you want to change the base?
Conversation
desired feature |
I'll also vote for that feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change looks good. How do other feel about it?
I am concerned bah the performance. Querying the backlog each time. Such
things can be done externally. For example by recording accepted requests
in a counter and reduce a backlog counter. IMO just maintenant a counter is
enough since the backlog setting is fixed.
On Tue 16 Feb 2021 at 03:57, Randall Leeds ***@***.***> wrote:
***@***.**** approved this pull request.
I think this change looks good. How do other feel about it?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#2407 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAADRIXB2KQEYLIABP6WUBDS7HNIFANCNFSM4QF3NICQ>
.
--
Sent from my Mobile
|
I don't think this can be done by recording accepting requests. The application cannot know how many requests are in the backlog without the OS telling it. This is happening in the arbiter, so once per second at the most, I think. I doubt that making a syscall to get a number from an OS struct is expensive, but it would be great if someone could chime in who knows a bit more about how this works than I do. |
Well 1 accepted request is done by 1 worker. You can sum it all. It would
be a good approximation imo of what could still be done. And raise an
alert. Coupled with system stats.
but what about adding a worker there and separate metrics in another module
to make it customisable?
On Tue 16 Feb 2021 at 07:28, Randall Leeds ***@***.***> wrote:
I don't think this can be done by recording accepting requests. The
application cannot know how many requests are in the backlog without the OS
telling it. This is happening in the arbiter, so once per second at the
most, I think. I doubt that making a syscall to get a number from an OS
struct is expensive, but it would be great if someone could chime in who
knows a bit more about how this works than I do.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#2407 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAADRIQLCA2NNC3G4FFNJBLS7IGADANCNFSM4QF3NICQ>
.
--
Sent from my Mobile
|
That tells you how many requests are in progress, not how many are waiting in the OS backlog. I think I have to remove my approval anyway, though. I don't know if this information is available. I can't find any documentation about |
The documentation is definitely scarce on this. I can provide a few references:
Also I the |
This looks correct. Thank you for doing the research. I think I was confused because the name suggests un-ACK'd messages, but it seems the kernel is re-using this field to have a different meaning for listening sockets than for connected sockets. I doubt the performance of a single system call per listener should worry us in the arbiter loop. That loop is not tight. It sleeps often. |
It'd be awesome to have this work out of the box and exposed as a metric in |
Certainly a very desired feature. Would love to know if this can be included in a release. |
@patrickmariglia why do you need it? Can you elaborate ? I still don’t really see the point of such metric. it is not really operational since you can’t change the value without restarting and well you know it will fails due to a socket error. The question is what do you do with this alarm? Keeping counters of accepted , in error and released requests may be more usefull to understand the pressure. Afterall the client should normally expect to fail and retry. Servers have limited resources that can’t be scaled indefinitely. We should add it imo. I think it is acceptable as an option but this feature must be cross platform and standard. Why does it target specifically linux? How to make it cross platform? |
edited comment above. |
Well, backlog metric - number of connections waiting for be answered, so, if that metric is above 0 - worth to check/fix number of workers/threads What I would desire even more then backlog - number of active/spare threads - in fact it is connected, as far as number of threads will reach maximum (workers x threads) - backlogs starts to grow. So, if you can monitor number of spare threads - you can forecast (more or less) when you will have no enough threads to process all parallel incoming connections. |
@vgrebenschikov explains very well what my use case is, I am trying to calculate or approximate worker saturation. Using socket backlog, or rather the number of waiting requests, can be a good proxy metric for worker saturation in situations where CPU is not a sufficient metric. So if this value even occasionally increases from 0 it could be an indicator that the number of workers may need to be increased or perhaps more replicas may need to be spun up (if you are in a k8s environment). If there is another way to determine saturation, such as number of spare workers as @vgrebenschikov also points out, that would be equally as useful. I had thought this possible with the |
Scaling up pods in a k8s environment with this metric could be a usecase. The saturation means that there's not enough workers to handle the requests. |
I've found the Also, it can explain discrepancies when our ingress in Kubernetes says a request to a Gunicorn app took 10 seconds, but Gunicorn says it took 1 second. In this example, the request spent 9 seconds in the Gunicorn backlog and only after that, when it was picked up by a Gunicorn worker, did Gunicorn start counting the request duration. Would really appreciate this PR getting merged. |
@benoitc , if I understand correctly, you would be willing to merge this feature if it was available on all platforms and not only linux or do you have other concerns or an alternate implementation you'd rather see merged? This metric is useful for us in understanding stalls in request processing as @matthew-walters and @israelbgf point out. |
Very desired feature! |
I still think this makes sense. I could imagine someone using this to decide when to scale up an autoscaling deployment. It does give a sense of whether the workers are able to process requests as fast as they arrive or not. Its utility may vary by worker type, but that's okay. I don't see the harm in the feature, unless we are worried about the extra overhead. If we are, we can make it optional, no? |
I agree, this feature would be very useful ! |
For folks that need this behavior, I think you can implement this locally by creating a |
I was looking to expose saturation metrics for a service my team's responsible for, and this metric combined with a metric on the # of workers that are actively handling a request would be an ideal combination. With the latter we would have an effective metric of saturation — e.g. at a moment in time, 4/5 workers are busy then we are at 80% saturation. This PR would then give us the impact of saturation — that is if we hit 5/5 workers a small backlog size would not be an issue, but a large one would tell us how severe the issue is. As of right now I can look at the difference in response times between the downstream load balancer and the gunicorn application server, but it's not ideal. |
There's a lack of metrics to scale gunicorn horizontally in k8s env now. This PR would definitely solve it! |
Hello good folks, |
This would be a very helpful metric for a scaling issue that we are running into. As such, I'm also interested in the timeline on the release. |
data point regarding safety: we've been running this in production for the last 60 days across ~500 instances of gunicorn without any issues ( |
I see PR is approved. Is it possible to include in next version. We are looking forward for this metrics. Thanks. |
Any updates or visibility regarding the merge/release timeline? This will be very helpful for a lot of folks looking to auto scale. Big thanks to all contributors 🙏 |
Still running this in production without issues, would love if it was merged and released. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR and feedbacks.
Let say t I am still not convinced this metric is useful compared its impact on the arbiter. Backlog is set statically once, so it's easy to extrapolate about its usage using the average connections or can be checked by comparing the number of request landing in the proxy with the number of running or accepted requests in gunicorn. That would reduce the contention there. Especially since this requires to decode a value.
That said, I understand that people may want to get it. so I suggest the following changes (see comment inlines):
- only trigger this metric on system that support it
- possibly make this metric configurable.
This would let the system perform as usual when it can. Can you make these changes? As for 2 this will be easier soon with the new opentelemetry backend but can be passed using the setting module for now.
If all the workers are busy or max connections is reached, new connections will queue in the socket backlog, which defaults to 2048. The `gunicorn.backlog` metric provide visibility into this queue, and give an idea on concurrency, and worker saturation. This also adds a distinction between the `timer` and `histogram` statsd metric types, which although treated the same, can be difference, for e.g. in this case histogram is not a timer: https://github.com/b/statsd_spec#timers
Fix failing lint tests
@benoitc I've updated the PR with the changes requested, please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest changes look good tor me, thank for them! Can you look at my comment, either way I think it's good for merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Sorry , I missed the backlock check in my previous review. Can you fix it as well? Also Look at the failing CI tests
gunicorn/arbiter.py
Outdated
backlog = sum(sock.get_backlog() or 0 | ||
for sock in self.LISTENERS) | ||
|
||
if backlog: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be always true there.even when backlog is set to -1.
I think correct test is :`if backlog >= 0'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I missed this - pushing the fix
02389e4
to
cf861a2
Compare
Hi @benoitc the CI has passed |
Won't this break if the connection is already submitted to the ThreadPoolExecutor in a gthread worker? if not, i think it's worth specifying it in the docs. |
@Barsoomx I didn't understand, can you elaborate? the backlog is from the sockets and isn't dependent on the number of worker connections. |
I've tried to implement this locally with gthread worker class and it doesn't show the correct backlog count. I suspect the reason is keepalive + worker_connections logic (sockets are ACKed) what happens in a sync worker: connections are in tcp backlog until worker can pick them up what happens in a threaded worker: the connections are enqueued into a threadpool and are being keepalived until a thread can process them, up to In that case (the threaded worker) the metric has no value and the real "worker backlog" is the connections that exceed the thread count for each worker.
|
Come on folks, it's been over 4 years now. |
If all the workers are busy or max connections are reached, new connections will queue in the socket backlog, which defaults to 2048. The
gunicorn.backlog
metric provides visibility into this queue and gives an idea on concurrency, and worker saturation. However, this is only available on Linux platforms.This also adds a distinction between the
timer
andhistogram
statsd metric types, which although treated the same, can be the difference, for e.g. in this case histogram is not a timer: https://github.com/b/statsd_spec#timersAlso, another point to note is on Linux the backlog is also limited by
net.core.somaxconn
which is 128 by default. Not sure if that is the case on other platforms as well. Would it then make sense to reduce the default backlog from 2048?Partially Fixes: #2057