feat(nuxt): Add Http responseHook with waitUntil

[s1gr1d]

With waitUntil the lambda execution continues until all async tasks (like sending data to Sentry) are done.

Timing-wise it should work like this: span.end() -> waitUntil() -> Nitro/Node response.end()

The problem in this PR was that the Nitro hook afterResponse is called to late (after response.end()), so waitUntil() could not be added to this hook.

---

Just for reference how this is done in Nitro (and h3, the underlying http framework):

1. The Nitro afterResponse hook is called in onAfterResponse
   https://github.com/unjs/nitro/blob/359af68d2b3d51d740cf869d0f13aec0c5e9f565/src/runtime/internal/app.ts#L71-L77

2. h3 onAfterResponse is called after the Node response was sent (and onBeforeResponse is called too early for calling waitUntil, as the span just starts at this point):
   https://github.com/unjs/h3/blob/7324eeec854eecc37422074ef9f2aec8a5e4a816/src/adapters/node/index.ts#L38-L47

   • endNodeResponse calls response.end(): https://github.com/unjs/h3/blob/7324eeec854eecc37422074ef9f2aec8a5e4a816/src/adapters/node/internal/utils.ts#L58

[andreiborza]

Any way we can add tests for this?

[andreiborza]

l: A bunch of SDKs reimplement this. Maybe we can think of hoisting it into utils? No strong feelings though.

[s1gr1d]

Good idea! But I'll create another PR for this change.

[andreiborza]

q: How about netlify and others? I wonder if we also want to flush in those cases?

[s1gr1d]

As far as I know Netlify does not expose a function like waitUtil 🤔 But it might make a difference if the SDK just flushes in the response hook - I would have to try.

[andreiborza]

Right, maybe we can try flushing here in case of serverless.

[mydea]

m: Can we add functionality to add this to our own SentryHttpInstrumentation instead? First, this will mean we do not rely on the otel http instrumentation for this. It also means that users may adde their own responseHook and it will not lead to problems.

[Lms24]

@mydea, @s1gr1d and I just talked about this and - while we agree it'd be good to eventually do this in the SentryHttpIntegration, I think it currently doesn't solve the core issue:

It also means that users may add their own responseHook and it will not lead to problems

Unfortunately, I think this isn't the case as long as both, Otel and Sentry HttpInstrumentation are in one httpIntegration. If users add their own httpIntegration with whatever customization they apply, it would override any httpIntegration instance we pass in. This is because user-defined integrations have precedence over defaultIntegrations.

In the Remix SDK, we currently handle this by adding a custom httpIntegration to defaultIntegrations which may get overridden if users provide their own httpIntegration in integrations. For now, I propose we handle this similarly in Nuxt (cc @s1gr1d, as discussed) and revisit how we can more easily ensure that a custom responseEnd hook is executed independently of what users provide. Does this make sense?

RE how can we do better? Maybe we can split up the otel and sentry instrumentations to be individual integrations? Then we can "special-case" the sentry one and add whatever hook to it in framework SDKs without running into the risk of it being overridden by users. WDYT?

[Lms24]

I think for now this is reasonable and unblocks the beta release. We can (and should) revisit a better way to add the responseHook in the future.

[mydea]

Unfortunately, I think this isn't the case as long as both, Otel and Sentry HttpInstrumentation are in one httpIntegration. If users add their own httpIntegration with whatever customization they apply, it would override any httpIntegration instance we pass in. This is because user-defined integrations have precedence over defaultIntegrations.

I was not talking about the httpIntegration, but about users providing their own OTEL HttpInstrumentation instance ;) This would overwrite the one we register in httpIntegration.

Nevertheless, it is a reasonable decision to do this now, let's just revisit this - overall, we should not depend on config passed to HttpInstrumentation for core functionality, and consider this (mentally) "optional". But we can iterate on this, as this is all internal :)