grafana / pyroscope-nodejs

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

Support Pull mode #4

Open eh-am opened 2 years ago

eh-am commented 2 years ago

From nodejs side we need

  1. Export basic profile collection data (cpu, heap), for people that want to add
  2. Maybe export a middleware for popular frameworks? Not entire sure how much work would be to support most popular frameworks, but worst case scenario people can implement themselves (using 1)
  3. Maybe follow go's pprof (https://pkg.go.dev/net/http/pprof) example and create a server ourselves? We could use a simple http server https://nodejs.org/en/knowledge/HTTP/servers/how-to-create-a-HTTP-server/
lizthegrey commented 2 years ago

I think this is done yeah?

RicardoMonteiroSimoes commented 1 year ago

I'm currently trying to implement Pyroscope into our nodeJs pods in production, and so far part of it seems to work according to the documentation. As we have a scrapper that collects this data, I'm setting it up in Pull mode, with the following setup:


Pyroscope.init({
  appName: 'app'
})
Pyroscope.startHeapProfiling()

router.get("/pprof/profile", Permissions.public, async (req, res) => {
  try {
    const p = await Pyroscope.collectCpu(0.01);
    res.send(p);
  } catch (e) {
    console.error('Error collecting cpu', e);
    res.sendStatus(500);
  }

});

router.get("/pprof/heap", Permissions.public, async (req, res) => {
  try {
    const p = await Pyroscope.collectHeap();
    res.send(p);
  } catch (e) {
    console.error('Error collecting memory', e);
    res.sendStatus(500);
  }
});

According to the docs this should be correct. The problem now is that collectHeap errors out:

2023-05-31 13:56:58 Error collecting memory Error: Heap profiler is not enabled.
2023-05-31 13:56:58     at v8Profile (/usr/src/app/node_modules/pprof/out/src/heap-profiler.js:32:15)

So, of course I thought "ah well, maybe I need to manually turn it on", which is why there is a line with Pyroscope.startHeapProfiling() in it. Apparently this triggers a specific check, and then requires there to be a server address:

throw 'Please set the server address in the init()';

If I set it, I can then just pull the data, but I suspect that it will try launching it's own server in the meantime. The culprit seems to be this codebit:

https://github.com/grafana/pyroscope-nodejs/blob/f0f5a777536fa010920e76c8112d3c62b5e04295/src/index.ts#L258-L270

It does not seem to respect if we are trying to use it in pure pull mode or not, and will blindly require a serverAddress. The doc here (why are there two?) https://github.com/grafana/pyroscope-nodejs#pull-mode does suggest that it does not need that parameter, and only needs appName when running in pull mode.

RicardoMonteiroSimoes commented 1 year ago

bump?

usermakename commented 1 year ago

i'v changed default path for memory heaps cause of scraping from grafana-agent without changing grafana-agent configuration (related to https://github.com/grafana/pyroscope-nodejs/issues/36) and starting HeapProfiling inside handler, init was just for pass the errors about appName and serverAddress in DEBUG=pyroscope i dont see any heap profiles sended to server, and they are scraped may be it's may be usefull for somebody :)

Pyroscope.init({
    serverAddress: 'localhost',
    appName: 'appName',
});
app.get('/debug/pprof/allocs', async function handler(req, res) {
    Pyroscope.startHeapProfiling();
    logger.info('Collecting memory');
    try {
        const p = await Pyroscope.collectHeap();
        res.send(p);
        Pyroscope.stopHeapProfiling();
    } catch (e) {
        logger.error('Error collecting memory', e);
        res.sendStatus(500);
    }
});