search

grafana / pyroscope-nodejs

Pyroscope NodeJS integration
Apache License 2.0
27 stars 21 forks source link

Making sampling Interval & Duration Configurable #22

Closed k4kratik closed 1 year ago

k4kratik commented 1 year ago

Adresses #21

I checked and found out these critical parameters are not configurable. Also, their name and use was very confusing.

So I renamed variable INTERVAL & SAMPLERATE to SAMPLING_INTERVAL_MS(default - 10ms) & SAMPLING_DURATION_MS(Default - 1000ms).

hope this helps. Shout out to @kolesnikovae🎖️ & @eh-am🎖️ for the help and support.

k4kratik commented 1 year ago

@kolesnikovae - Do we need to reduce the default value of SAMPLING_DURATION_MS ? I tried with 1000ms and all test passed.

k4kratik commented 1 year ago
  1. RE: tests, I think with approach # 1 you can just pass a custom interval to Pyroscope.init. But in any case, I think it's still possible to overwrite process.env vars dynamically before starting the test.

Hi @eh-am, thanks for the review. I ran test with SAMPLING_DURATION_MS 1000ms and it passed all tests.

k4kratik commented 1 year ago

.

k4kratik commented 1 year ago

Thank you @kolesnikovae 👍🏻