-
Notifications
You must be signed in to change notification settings - Fork 149
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
Consider exporting unaggregated per-pid metrics by default #107
Comments
Right, I've been digging through all the relevant code and doing some thinking on which approach to take. Looking at the aggregations supported by the Python client (scroll down to 'Three: Instrumentation'), I think we should start by supporting the 'all' mode. The 'liveall' and 'livesum' modes sound interesting and potentially useful, but I'm confident we can add them later with no breaking changes (i.e. I'm not making them part of the path to 1.0). My plan is to:
I'm happy to discuss any of those choices and change tack if there's a good reason to. |
Fwiw, providing a pid label has been the only legitimate way I've found to use gauges with the multi-process library. Reckon it's the only semantically valid approach! |
We use a |
@SuperQ That does sound way better! Out of curiosity, do you grab the worker number in I don't think we can do that for people in the gem as it would require us to hook into Unicorn (and Puma?) somehow. We could support changing the default aggregation for a given metric type and adding some docs for Unicorn/Puma users. If I've missed an approach that would be better, please shout! |
We have a helper for this. You can see how we use it here. |
Status update: the first and third bullet points from this comment are done. I'm going to try and get the second one done tomorrow, but my day is pretty packed so it might be next week now. @SuperQ Thanks for sharing those links! I'm going to have a think about whether there's an approach we can reasonably give people for free in this gem. The constraints I'm trying to make it fit into are:
|
I don't think it's wise to ever change the label for the pid/worker, as not only would you have to change your own code that happens to conflict but also that of every library that you use. Python always uses |
Yeah that makes total sense. Globally reserving more and more label names feels a tad ridiculous. Let's always use In which case, adding the Unicorn worker ID support later isn't a problem. I'm going to put together a PR that makes unaggregated per-process gauges the default and once that's merged this issue is done. |
We had a couple of comments on this point, and it’s fair. The point about “no obvious right way to aggregate gauges in all cases” is a solid argument.
We need to decide a stance on this both for gauges specifically and for metrics in general (i.e. should gauges be a special case where we don't aggregate by default).
This issue should be renamed and updated once we decide that stance.
The text was updated successfully, but these errors were encountered: