search

grafana / pyroscope-nodejs

Pyroscope NodeJS integration
Apache License 2.0
26 stars 18 forks source link

Add support for Windows #87

Open bryanhuhta opened 2 months ago

bryanhuhta commented 2 months ago

Currently, we don't support Windows because of our usage of dynamic labels. The DataDog profiler doesn't support dynamic labels on Windows:

https://github.com/DataDog/pprof-nodejs/blob/e7d3e053d9816e64710533738ff827c0301b82fe/bindings/profilers/wall.cc#L33

The DD_WALL_USE_SIGPROF enables dynamic labels and if it is disabled, it throws an error here:

https://github.com/DataDog/pprof-nodejs/blob/e7d3e053d9816e64710533738ff827c0301b82fe/bindings/profilers/wall.cc#L609-L611

We always enable withContexts in or SDK:

https://github.com/grafana/pyroscope-nodejs/blob/e2d157cf22f931b27e28e5cbe9ec631353c44360/src/profilers/wall-profiler.ts#L88

We might be able to support Windows by disabling this configuration when we detect a Windows host, or by providing a way for users to turn it off. At the very least, we should provide a gentler failure case which is more helpful for users.

See https://github.com/grafana/pyroscope-nodejs/issues/85 for an example of the SDK not working on Windows.

simonswine commented 2 months ago

A good first step might also be to not break the application when running on windows, even if the profiler doens't work/run at all.

I also think we probably should have caught that ourselves by having a test for other OSes than linux (relates to #86)