search

grafana / pyroscope-nodejs

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

`PYROSCOPE_SAMPLING_DURATION` is never used #27

Closed eh-am closed 1 year ago

eh-am commented 1 year ago

The env var is assigned here https://github.com/pyroscope-io/pyroscope-nodejs/blob/09d3f4d1b77d8640181e871db0fa3d60919e4a7d/src/index.ts#L30

And then it's used here https://github.com/pyroscope-io/pyroscope-nodejs/blob/09d3f4d1b77d8640181e871db0fa3d60919e4a7d/src/index.ts#L204

But the expression seems wrong

(seconds || 10) * 1000 || Number(SAMPLING_DURATION_MS)

Since (seconds || 10) will default to 10, which is then multiplied by 1000, which is truthy. Therefore it never defaults to whatever SAMPLING_DURATION_MS is.

eh-am commented 1 year ago

NVM there are 2 different code paths: https://github.com/pyroscope-io/pyroscope-nodejs/blob/09d3f4d1b77d8640181e871db0fa3d60919e4a7d/src/index.ts#L195 https://github.com/pyroscope-io/pyroscope-nodejs/blob/09d3f4d1b77d8640181e871db0fa3d60919e4a7d/src/index.ts#L249