Feature request: proper full hapi support

[AdriVanHoudt]

I know that express and koa are supported but it would be nice if hapi would also be supported out of the box.
I think this is mostly about proper handling of request data.
I think the express integration is exposed as middleware, you could do the same for hapi, there is called a plugin.

[lobsterkatie]

Thanks for the suggestion! Our roadmap for Q2 is pretty set, but I've added it to our backlog as something to consider in the future. In the meantime, we're always open to contributions from the community!

[AdriVanHoudt]

Hi @lobsterkatie,

I think a good start would be to take a quick look at https://github.com/hydra-newmedia/hapi-sentry and see if that implementation makes sense and is not missing anything obvious.

It is missing tracing possibilities though.
After a bunch of testing and looking at the express implementation I came up with this to test:

    server.ext('onRequest', (request, h) => {

        // Based on https://github.com/getsentry/sentry-javascript/blob/533975107d9978bcae0b78f61ac214dff4e9ad14/packages/node/src/handlers.ts#L50
        const transaction = Sentry.startTransaction({
            op: 'http.server',
            name: `${request.method.toUpperCase()} ${request.path}`
        }, {
            request: Sentry.Handlers.extractRequestData(request.raw.req)
        });

        Sentry.configureScope((scope) => {

            scope.setSpan(transaction);
        });

        request.app.sentry = {
            transaction
        };

        return h.continue;
    });
    server.events.on('response', (request) => {

        request.app.sentry.transaction.name = `${request.method.toUpperCase()} ${request.route.path}`; // Set parameterized as transaction name e.g.: `GET /users/{id}`
        request.app.sentry.transaction.setHttpStatus(request.response.statusCode);
        request.app.sentry.transaction.setData('url', request.path);
        request.app.sentry.transaction.setData('query', request.query);
        request.app.sentry.transaction.finish();
    });

I think it might warrant its own issue but a big part of what is missing is the use of Async Hooks.
Using domains is deprecated and comes with a lot of edge cased and performance implications.
Not using Async Hooks basically makes the tracing unusable since it never matches the db calls to the right request properly for example.
If you want I can share some tracing + async hook examples that might be handy as examples.

I've been in contact with your support as well so let me know where I best keep communicating.

[lobsterkatie]

This is the best place to continue the conversation.

We currently still support node 6, which doesn't have async hooks, so for the moment at least, we can't do that. I would be curious to see how you're envisioning using them for this, though.

And while I grant that the use of domains is deprecated, I checked and it's been deprecated ever since node 4, and we're now on node 16. Domains are still here, we know they work for our use case and are likely not changing, and we've yet to find an alternative we think is better. If you think async hooks might be that alternative in the future (after we drop node 6 support someday), I'm interested to hear about it. For the moment, though, we're going to stick with domains.

[AdriVanHoudt]

This reply reads very dismissive (which I hope it is not).

While I don't think you should still support node 6 for various reasons (support, security, ...) you probably (hopefully) have good business reasons for this, so I won't argue that point.

Sadly this impacts every other customer not on node 6 (which I hope is the majority).
This means I have to choose between correct and usable data or performance. Which isn't a great tradeoff.

It would be nice if the SDK would detect the node version and use the best possible solution for that version. So domains on node 6 and hooks on node 8 (might need to be 8.5 or something).

You state that domains code won't change while it did in (I believe) node 10. See nodejs/node#16222.
That PR is interesting since it changes domains to use hooks underneath. Which is great but still results in worse perf than just using hooks and it will only get worse over time as it won't be optimized or won't benefit from optimizations in hooks. I talked to the author of that PR to check whether it was an option. Which is where I got a bunch of the info mentioned.

You also stated you know domains will work as long as they exist, which is true I guess but this is also the case for hooks? Hooks might actually receive some changes in the future but those would be to make things easier or more performant. The reason domains are not gone yet is because of userland usage. I don't see node dropping hooks for the same reason. It would break almost all apm agents at this point.

I would be curious to see how you're envisioning using them for this, though.

I might be misreading your point but if you want to see how to use hooks for traces:

• https://github.com/googleapis/cloud-trace-nodejs
• https://github.com/honeycombio/beeline-nodejs
• https://github.com/elastic/apm-agent-nodejs
• https://github.com/DataDog/dd-trace-js/tree/master/packages/dd-trace

[lobsterkatie]

I apologize - that was certainly not my intent. Full disclosure, the question of whether or not to continue using domains has come up many times before, and often turned into a lot of back and forth, and wanting to avoid that is probably what you were hearing. But that's not on you, so again, apologies!

And I have to hand it to you - you have done your research! I will bring this back to the team to see what they think. While there's enough on our docket that we're unlikely to make any independent changes here in the short term, we do have an upcoming goal of re-evaluating our performance API, so perhaps this could be rolled into that. I'll add it in as a note in our planning doc. Thanks.

[AdriVanHoudt]

we do have an upcoming goal of re-evaluating our performance API

This doesn't only impact the performance api. This also impacts things like breadcrumbs on errors.

I do think that not having proper tracing with async hooks out of the box makes the Sentry agent way less interesting.
The fact that hapi specifically is not supported out of the box is fine (it doesn't have the biggest ecosystem so I get that). But that I have to look at the internals of the sdk to figure how to add this myself and then figure out that the agent doesn't do proper tracing out of the box and then find out you recommend domains is not the best experience.

I think those are things that are easily fixable by just clearly stating them in the docs (which are otherwise very nice and clear in general).

I'm happy to talk further about how the tracing could look or how to best handle adding hapi support (or officially endorsing https://github.com/hydra-newmedia/hapi-sentry or something)

[AdriVanHoudt]

I've been testing to see if we can use the domain based solution until the agent catches up.
But either I'm doing something wrong or something is not properly working.

I'm still seeing db spans being thrown together on 1 transaction instead of properly on each transaction.
Which results in data like this (notice the 1 transaction with all the db spans and the other transaction have nothing):

We do use hapi-plugin-mysql which promisifies the query method (https://github.com/Salesflare/hapi-plugin-mysql/blob/c9ebd713ef639ffd9f4779757c98dc1b8e4407eb/lib/index.js#L19).
But the spans it does collect seems correct so it seems like a context binding issue.
It also seems to be doing the correct thing when stepping through it with the debugger but I'm not familiar with the sdk internals so I could be missing something.
It only seems to happen under higher load as well (using autocannon with default settings to test load). Testing a 1 off request seems fine. Which seems like it starts failing when doing more than 1 request at the same time.

This is my current test code:

server.ext('onRequest', (request, h) => {

        request.app.sentry = {};

        // Domain stuff based on https://github.com/hydra-newmedia/hapi-sentry/blob/283f0f8966c03d8a1f5a4b3a7a2709ba3b93858e/index.js#L41

        // Sentry looks for current hub in active domain
        // Therefore simply by creating and entering domain Sentry will create
        // request scoped hub for breadcrumbs and other scope metadata
        request.app.sentry.domain = Domain.create();
        request.app.sentry.domain.enter();

        // Based on https://github.com/getsentry/sentry-javascript/blob/533975107d9978bcae0b78f61ac214dff4e9ad14/packages/node/src/handlers.ts#L50
        const transaction = Sentry.startTransaction({
            op: 'http.server',
            name: `${request.method.toUpperCase()} ${request.path}`
        }, {
            request: Sentry.Handlers.extractRequestData(request.raw.req) // Passed to transaction sampling function
        });

        Sentry.configureScope((scope) => {

            scope.setSpan(transaction);
        });

        request.app.sentry.transaction = transaction;

        return h.continue;
    });
	// Same result with `server.ext('onPostAuth'`, lifecycle ref https://futurestud.io/files/hapi/hapi-request-lifecycle.pdf
    server.events.on('response', (request) => {

        request.app.sentry.transaction.name = `${request.method.toUpperCase()} ${request.route.path}`; // Set parameterized as transaction name e.g.: `GET /users/{id}`
        request.app.sentry.transaction.setHttpStatus(request.response.statusCode);
        request.app.sentry.transaction.setTag('http.method', request.method.toUpperCase()); // Attempt to set the http method on the performance dashboard, but it seems like it only attaches it when an error happens. So probably not the correct way.
        request.app.sentry.transaction.setTag('http.url', request.path); // For filtering, although the dashboard doesn't seem to like it atm
        request.app.sentry.transaction.setData('url', request.path);
        request.app.sentry.transaction.setData('query', request.query);
        request.app.sentry.transaction.finish();

        // Exiting domain
        request.app.sentry.domain.exit();
    });

	// not exact but technically the same
    server.ext('onPostAuth', (request, h) => {

        // Set user info for Sentry
        Sentry.configureScope((scope) => {

            scope.setUser({ ...request.auth.credentials });
        });
		return h.continue;
	});

Am I missing something obvious or doing something wrong?

[AdriVanHoudt]

Hi,

I made a general issue for the async hooks over here: #3660 as it seemed to sidetrack this issue, which is about hapi support.

I'm still looking forward to a reply, hoping I messed something up so we can actually use Sentry for performance tracing.

[AbhiPrasad]

Tracking in #6973 - closing this issue until we start work on it.