process_xxx_memory statistics for macOS (cgo) #1616
Conversation
mharbison72
force-pushed
the
mac-process-mem-stats
branch
from
78af2fb
to
eca836e
Compare
|
As an alternative to this, I have now managed to also get it working without cgo, using The one wrinkle that I noticed is that the command code passed to the native API is a macro that evaluates to 5 on amd64, and 18 on arm64. It's this kind of nuisance stuff that gives me pause using this mechanism, though there's clear benefit instead of silently dropping some metrics. Maybe a reasonable solution is using So I guess the choices are simpler code, but slightly limited (this cgo PR), or a dependency + a bit more code without C compiler checks by default, but more flexible for users. Let me know if this second approach is reasonable, and I can clean up and submit this alternative in a different PR. |
mharbison72
changed the title
|
Epic! I would love to avoid Personally I would rather have something optional, perhaps requirement for building with CGO IF you want darwin RSS (otherwise it will not expose that metric) would be ok. If we want something more complex we might want to put process collector as a separate Go module/project (: |
Totally understandable, and that's why I opted for cgo first. FTR, the purego version is #1629 (though I used a newer structure that better supports arm64, so this code needs to be updated too).
Can you explain? In my mind, that's how this code works- if Are you suggesting an explicit build tag? One of the reservations expressed in the IRC channel about cgo was people needing to update their build scripts to use it, and it would seem like an explicit build tag would have the same issue. I like how this is able to avoid that concern (though it's almost too magical in that when I make a fat binary by compiling both architectures, only one has
I would like to keep this as simple as possible. I'm not sure if you're suggesting another module in this repo (or another GH/prometheus repo), but that will probably require more knowledge of the project than I have. |
prometheus/process_collector.go
Outdated
| @@ -33,6 +33,14 @@ type processCollector struct { | |||
| inBytes, outBytes *Desc | |||
| } | |||
|
|
|||
| func init() { | |||
| // Debugging to show the correct implementation is used based on CGO_ENABLED=0. | |||
There was a problem hiding this comment.
Do we want to merge that though?
There was a problem hiding this comment.
Do we want to merge that though?
No. This initial PR was just throwing a dart to show it working, and get either a "this cgo is bad, please avoid it", or "3rd party deps are bad, use cgo", or "this isn't workable at all" response to resolve the various discussions about #1590.
Let me review deeper, is this ready to go then?
It sounds like you're agreeable to the general idea here, so give me a day or two to migrate this to the APIs that I used in the purego version for portability. Is there a preferred format for C code, or is anything fine as long as it's reasonable?
No, this would work fine I guess, it's what I meant. Let me review deeper, is this ready to go then? |
prometheus#1600) Unfortunately, these values aren't available from getrusage(2), or any other builtin Go API. Go itself doesn't provide a mechanism (like on Windows) to call into system libraries. Using a 3rd party package[1] to dynamically call system libraries was proposed and rejected, to avoid adding to the number of dependencies. That leaves using cgo, which is used here when available. When not available (either because of cross compiling or explicitly disabling it), a stub function is linked instead, and the metrics are not exported. That way, cross compiling of other platforms is unaffected (and can also still be done with Darwin too, but at the cost of not exporting these metrics). Note that building an amd64 image on an arm64 mac or vice-versa is cross compiling, and will use the stub method by default. This can be avoided by setting `CGO_ENABLED=1` in the environment to force the use of cgo for both architectures. I'm unsure of the usefulness of the potential adjustment made to the virtual memory value after calling `mach_vm_region()`. I've not seen that code get run with a native amd64 or arm64 image, or with an amd64 image running under Rosetta. But that's what the `ps(1)` command does, and I think we should report what the system tools do. When I was testing this on a beta of macOS 15 with Go 1.21.13 (the current minimum support for this module), the amd64 image ran fine under Rosetta, but the arm64 image immediately printed a message that it was killed, even prior to the cgo call. This seems to be a recurring issue on macOS[2][3], and passing `-ldflags -s` to `go build` avoided the issue. Go 1.23.1 worked out of the box, without fiddling with linker flags, so I don't think this is an issue- Go 1.21 is simply too old to support macOS 15, but I thought it was worth noting. I supposed we could gate the cgo code with an additional build flag, if anyone is concerned about this. [1] https://github.com/ebitengine/purego [2] golang/go#19841 (comment) [3] golang/go#11887 (comment) Signed-off-by: Matt Harbison <mharbison72@gmail.com>
mharbison72
force-pushed
the
mac-process-mem-stats
branch
2 times, most recently
from
0bbaeea
to
6619b9d
Compare
|
@bwplotka - Now it's ready for review. I'll close the purego one when this is closed out. Assuming this technique is ok, the network send/receive stats are the only two metrics missing for parity with Linux. It looks like the changelog is mostly managed outside of code changes, but LMK if you want me to change it in the next round (or a followup PR if this is acceptable). Needing to enable cgo explicitly when making fat binaries is probably sneaky enough to be worthy of calling out. |
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // |
There was a problem hiding this comment.
can we rename this file to process_collector_cgo_darwin.c too?
mharbison72
force-pushed
the
mac-process-mem-stats
branch
2 times, most recently
from
7c9e084
to
1a14ac7
Compare
|
Sorry about that- I screwed up the copy/paste and trimmed the "prometheus/" directory off. It should be as intended now. |
This is built on top of the much easier to get process metrics in PR1600, so only the last commit is interesting here (and the commit message has more details). I don't expect or want this to be landed as-is, but am posting it here to help with any discussion about finishing off #1590.
I've not paid much attention to the RSS values of Go processes on macOS, but after ~5 minutes of banging on my own exporter with
curl, I only ever see the value going up. The values agree with thepscommand. There should be ~0 code other than from this module running, but we'll see what happens.@bwplotka