grafana / pyroscope-rs

Pyroscope Profiler for Rust. Profile your Rust applications.
Apache License 2.0
132 stars 22 forks source link

Support Python on Windows #67

Open mjmdavis opened 1 year ago

mjmdavis commented 1 year ago

I'd like to use pyroscope for Python on Windows.

mjmdavis commented 1 year ago

So, I have a windows build running. I will test it later today.

I have two platform dependent errors that I had to work around:

Process ID is platform dependent

The pid type is set to i32 in pyroscope-rs: https://github.com/pyroscope-io/pyroscope-rs/blob/66b592b83ca245ed041c2160fada4595fcdf4a4d/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L32 https://github.com/pyroscope-io/pyroscope-rs/blob/66b592b83ca245ed041c2160fada4595fcdf4a4d/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L69

However, in py-spy, the pid type can be either i32 or u32 depending on the platform it's built on. This causes a type error when compiling pyroscope-rs for windows.

The source of this is a transitive dependency via benfred/py-spy on benfred/remoteprocess: https://github.com/pyroscope-io/pyroscope-rs/blob/66b592b83ca245ed041c2160fada4595fcdf4a4d/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L251-L255 https://github.com/benfred/py-spy/blob/538a2c63327adc9c0b921e1502c6e9156b1e14bb/src/config.rs#L22 https://github.com/benfred/py-spy/blob/538a2c63327adc9c0b921e1502c6e9156b1e14bb/src/config.rs#L2

The Pid type returned by remoteprocess for unixy platforms is i32, defined in rust-lang/libc via this chain: https://github.com/benfred/py-spy/blob/538a2c63327adc9c0b921e1502c6e9156b1e14bb/src/config.rs#L2 https://github.com/benfred/remoteprocess/blob/9ca4be545f46014412f86827c87589cebc621ae1/src/osx/mod.rs#L14 https://github.com/rust-lang/libc/blob/15d27952bfa93e5e4f419c603f275486f15a050c/src/unix/mod.rs#L25

For windows it's u32 on this chain: https://github.com/pyroscope-io/pyroscope-rs/blob/66b592b83ca245ed041c2160fada4595fcdf4a4d/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L254 https://github.com/benfred/py-spy/blob/538a2c63327adc9c0b921e1502c6e9156b1e14bb/src/config.rs#L2 https://github.com/benfred/remoteprocess/blob/9ca4be545f46014412f86827c87589cebc621ae1/src/windows/mod.rs#L12 https://github.com/rbspy/read-process-memory/blob/d0aeb5088ec5df2b383df02f8772a33b484f1f81/src/lib.rs#L396 https://github.com/retep998/winapi-rs/blob/0.3/src/shared/minwindef.rs#L20 https://github.com/retep998/winapi-rs/blob/2f76bdea3a79817ccfab496fbd1786d5a697387b/src/lib.rs#L47

libc::pthread_self() is not portable to windows

Less complicated to fix this function but perhaps more complicated to get the functionality to work.

https://github.com/pyroscope-io/pyroscope-rs/blob/70ff908cb84a4b15fefcf52275c1ca1bbb72569a/src/utils.rs#L76-L80

mjmdavis commented 1 year ago

To address the pthread_self() issue, I added: https://github.com/pyroscope-io/pyroscope-rs/blob/d03601a25cb2daab515303dcbdd650643c458c95/src/utils.rs#L77-L88

mjmdavis commented 1 year ago

For the pid type issue, there are a few options. For now, I added a direct dependency on remoteprocess to pyroscope_pyspy: https://github.com/pyroscope-io/pyroscope-rs/blob/d03601a25cb2daab515303dcbdd650643c458c95/pyroscope_backends/pyroscope_pyspy/Cargo.toml#L21

This allows the use of the type determined at build time in that library: https://github.com/pyroscope-io/pyroscope-rs/blob/d03601a25cb2daab515303dcbdd650643c458c95/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L17 https://github.com/pyroscope-io/pyroscope-rs/blob/d03601a25cb2daab515303dcbdd650643c458c95/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L70 https://github.com/pyroscope-io/pyroscope-rs/blob/d03601a25cb2daab515303dcbdd650643c458c95/pyroscope_backends/pyroscope_pyspy/src/lib.rs#L68-L75

An alternative might be to get this type from read-process-memory which is already used in remoteprocess. However, remoteprocess only gets the pid type from there for windows builds. For other platforms, it seems to use libc directly instead of using read-process-memory. One might argue that it should use read-process-memory for consistency.

Another alternative would be for py-spy to re-export the Pid type so that extra dependencies are not needed.

mjmdavis commented 1 year ago

Well, it seems that py-spy does re-export the Pid type. So I'm a dummy.

mjmdavis commented 1 year ago

So, it works. I was able to send metrics to the pyroscope cloud using an example program.

korniltsev commented 1 year ago

I am having trouble to make tests working on windows

 2023-01-16T08:05:49.106Z INFO  Pyroscope::Session > Creating Session
 2023-01-16T08:05:49.106Z INFO  Pyroscope::Session > Creating Session
 2023-01-16T08:05:49.106Z INFO  Pyroscope::Session > Creating Session
 2023-01-16T08:05:49.106Z INFO  Pyroscope::Session > Creating Session
 2023-01-16T08:05:49.107Z INFO  py_spy::python_spy > Found libpython binary @ C:\hostedtoolcache\windows\Python\3.10.9\x64\python310.dll
 2023-01-16T08:05:49.111Z INFO  py_spy::python_spy > Getting version from python binary BSS
 2023-01-16T08:05:49.111Z INFO  py_spy::python_spy > Failed to get version from BSS section: failed to find version string
 2023-01-16T08:05:49.111Z INFO  py_spy::python_spy > Getting version from libpython BSS
 2023-01-16T08:05:49.112Z INFO  py_spy::version    > Found matching version string '3.10.9 (tags/v3.10.9:1dd9be6, Dec  6 2022, 20:01:21) [MSC v.1934 64 bit'
 2023-01-16T08:05:49.112Z INFO  py_spy::python_spy > python version 3.10.9 detected
 2023-01-16T08:05:49.112Z INFO  py_spy::python_spy > got symbol _PyRuntime (0x00007ffa60f3e000) from libpython binary
 2023-01-16T08:05:49.112Z WARN  py_spy::python_spy > Interpreter address from _PyRuntime symbol is invalid 003cb6e000173831
 2023-01-16T08:05:49.112Z INFO  py_spy::python_spy > Failed to get interp_head from symbols, scanning BSS section from main binary
 2023-01-16T08:05:49.113Z INFO  py_spy::python_spy > Failed to get interpreter from binary BSS, scanning libpython BSS
 2023-01-16T08:05:22.606Z INFO  py_spy::python_spy > Failed to connect to process, retrying. Error: Failed to find a python interpreter in the .data section

I am trying the test from the branch https://github.com/pyroscope-io/pyroscope-rs/pull/76 , but on master it also does not work

The job https://github.com/pyroscope-io/pyroscope-rs/actions/runs/3928207781/jobs/6715641552

@mjmdavis Would you like to take a look? If no, I will try to do it later some time.