ref(node)!: Remove legacy incoming HTTP span hooks and default keepAlive to true - #23396
ref(node)!: Remove legacy incoming HTTP span hooks and default keepAlive to true#23396RulaKhaled wants to merge 5 commits into
Conversation
…ive to true Incoming request spans now only go through `onSpanCreated` / `incomingRequestSpanHook`. The HTTP transport reuses sockets by default now that Node 8 keepAlive leaks are out of support. Fixes #22260 Co-Authored-By: Cursor Grok 4.6 <cursoragent@cursor.com>
size-limit report 📦
|
CHANGELOG and root MIGRATION.md are maintained separately; the breaking-change write-up for this PR lives in docs/migration/v11-end-state.md.
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 044bf40. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
This looks really good. I think there's some more clean-up that could potentially be done, but wouldn't block on that. The only real issue is that keepAlive can be potentially hazardous without a retry for serverless use cases.
| * These no longer run for incoming request spans; use `incomingRequestSpanHook` for those. | ||
| */ | ||
| instrumentation?: { | ||
| requestHook?: (span: Span, req: HttpIncomingMessage | HttpClientRequest) => void; |
There was a problem hiding this comment.
low/cleanup: These are now only for outgoing requests now, but the types still accept HttpClientRequest for the request hook and HttpIncomingMessage for the response hook. The types can probably be cleaned up, since it's more restrictive now.
There was a problem hiding this comment.
Already handled in the stacked PR
| */ | ||
| trackIncomingRequestsAsSessions?: boolean; | ||
|
|
||
| /** |
There was a problem hiding this comment.
low/cleanup: we can probably just remove the rest of these "deprecated: does nothing" options.
There was a problem hiding this comment.
also handled in the stacked PR
| @@ -106,14 +106,12 @@ function getNuxtDefaultIntegrations(options: NodeOptions): Integration[] { | |||
| ...getDefaultNodeIntegrations(options).filter(integration => integration.name !== 'Http'), | |||
| // The httpIntegration is added as defaultIntegration, so users can still overwrite it | |||
| httpIntegration({ | |||
There was a problem hiding this comment.
low/cleanup: I think we can just delete this override now. All it's doing is calling flushIfServerless(), but that's already being called by packages/nuxt/src/runtime/utils/patchEventHandler.ts. Also, this is redundant anyway, because it's being called at the start of the span, before we have anything to flush. Then we can also remove the integration.name !== 'Http' filter on the line above.
There was a problem hiding this comment.
will land in the stacked PR
|
|
||
| // TODO(v11): Evaluate if we can set keepAlive to true. This would involve testing for memory leaks in older node | ||
| // versions(>= 8) as they had memory leaks when using it: #2555 | ||
| const keepAlive = options.keepAlive ?? true; |
There was a problem hiding this comment.
The node 8 memory leaks are not an issue, but I think this might have some further reaching consequences, since this transport is also used for aws-serverless and google-cloud-serverless use, so it's possible that the socket times out while the process is frozen. (Also, this is ignored for the proxy case.)
It's not going to fail often, but I think we'd need to add a retry in makeRequest to make sure that if we get a dead socket, we don't crash on it.
Eg, wrap the contents of makeRequest in an async sendRequest = (canRetry: boolean) => {...}, and then have it do:
req.on('error', error => {
if (canRetry && req.reusedSocket && (error as { code?: string }).code === 'ECONNRESET') {
resolve(sendRequest(false));
} else {
reject(error);
}
});There was a problem hiding this comment.
Nice catch! will add the keepAlive reused-socket retry
…nsubscribe Keep-alive sockets can die while a serverless isolate is frozen; retry once when Node reports a reused socket reset. setupOnce is unique by name, so the diagnostics-channel unsubscribe is unused. Co-Authored-By: Cursor Grok 4.6 <cursoragent@cursor.com>
2 participants
Incoming
http.serverspans only useincomingRequestSpanHook/onSpanCreated.httpIntegration'sinstrumentation.*hooks no longer run for incoming requests; they still apply to outgoing. The Node HTTP transport now defaultskeepAlivetotrue.TODOs:
requestHook/responseHook/applyCustomAttributesOnSpan; keeponSpanCreated— done.instrumentHttpOutgoingRequests()is still public and can be called more than once;subscribe()stacks.keepAlivetotrue— done. The leak was Node 8 era, v11 requires 20.19.0+.Fixes #22260