Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Pull mode #4

Open
eh-am opened this issue Apr 14, 2022 · 4 comments
Open

Support Pull mode #4

eh-am opened this issue Apr 14, 2022 · 4 comments

Comments

@eh-am
Copy link
Collaborator

eh-am commented Apr 14, 2022

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
Copy link

I think this is done yeah?

@RicardoMonteiroSimoes
Copy link

RicardoMonteiroSimoes commented May 31, 2023

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:

function checkConfigured() {
if (!config.configured) {
throw 'Pyroscope is not configured. Please call init() first.'
}
if (!config.serverAddress) {
throw 'Please set the server address in the init()'
}
if (!config.appName) {
throw 'Please define app name in the init()'
}
}

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
Copy link

bump?

@usermakename
Copy link

i'v changed default path for memory heaps cause of scraping from grafana-agent without changing grafana-agent configuration (related to #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);
    }
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants