-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
ref(node)!: Remove legacy incoming HTTP span hooks and default keepAlive to true #23396
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,7 +141,8 @@ interface HttpOptions { | |
| disableIncomingRequestSpans?: boolean; | ||
|
|
||
| /** | ||
| * Additional instrumentation options that are passed to the underlying HttpInstrumentation. | ||
| * Hooks for outgoing HTTP request spans. | ||
| * These no longer run for incoming request spans; use `incomingRequestSpanHook` for those. | ||
| */ | ||
| instrumentation?: { | ||
| requestHook?: (span: Span, req: HttpIncomingMessage | HttpClientRequest) => void; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. low/cleanup: These are now only for outgoing requests now, but the types still accept |
||
|
|
@@ -174,8 +175,6 @@ export const httpIntegration = defineIntegration((options: HttpOptions = {}) => | |
| ignoreIncomingRequests: options.ignoreIncomingRequests, | ||
| ignoreStaticAssets: options.ignoreStaticAssets, | ||
| ignoreStatusCodes: options.dropSpansForIncomingRequestStatusCodes, | ||
| // oxlint-disable-next-line typescript/no-deprecated -- pass through the deprecated option for back-compat | ||
| instrumentation: options.instrumentation, | ||
| onSpanCreated: options.incomingRequestSpanHook, | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ export interface NodeTransportOptions extends BaseTransportOptions { | |
| caCerts?: string | Buffer | Array<string | Buffer>; | ||
| /** Custom HTTP module. Defaults to the native 'http' and 'https' modules. */ | ||
| httpModule?: HTTPModule; | ||
| /** Allow overriding connection keepAlive, defaults to false */ | ||
| /** Allow overriding connection keepAlive, defaults to true */ | ||
| keepAlive?: boolean; | ||
| } | ||
|
|
||
|
|
@@ -68,10 +68,7 @@ export function makeNodeTransport(options: NodeTransportOptions): Transport { | |
| ); | ||
|
|
||
| const nativeHttpModule = isHttps ? https : http; | ||
| const keepAlive = options.keepAlive === undefined ? false : options.keepAlive; | ||
|
|
||
| // 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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 req.on('error', error => {
if (canRetry && req.reusedSocket && (error as { code?: string }).code === 'ECONNRESET') {
resolve(sendRequest(false));
} else {
reject(error);
}
}); |
||
| const agent = proxy | ||
| ? (new HttpsProxyAgent(proxy) as http.Agent) | ||
| : new nativeHttpModule.Agent({ keepAlive, maxSockets: 30, timeout: 2000 }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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({ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. low/cleanup: I think we can just delete this override now. All it's doing is calling |
||
| instrumentation: { | ||
| responseHook: () => { | ||
| // Flush eagerly on serverless platforms, where the function may be frozen before the transport | ||
| // sends, handing the flush to a platform `waitUntil` where one exists so it doesn't block. On a | ||
| // long-running server this is a no-op, so pending outcomes keep aggregating on the flush interval | ||
| // instead of shipping one client_report envelope per response. | ||
| void flushIfServerless(); | ||
| }, | ||
| incomingRequestSpanHook: () => { | ||
| // Flush eagerly on serverless platforms, where the function may be frozen before the transport | ||
| // sends, handing the flush to a platform `waitUntil` where one exists so it doesn't block. On a | ||
| // long-running server this is a no-op, so pending outcomes keep aggregating on the flush interval | ||
| // instead of shipping one client_report envelope per response. | ||
| void flushIfServerless(); | ||
|
RulaKhaled marked this conversation as resolved.
|
||
| }, | ||
| }), | ||
| ]; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context: #22260 said this was likely removable once @sentry/node/preload went away, but instrumentHttpOutgoingRequests() is still public and can be called more than once (last call wins). subscribe() stacks, so without unsubscribing first a second call would duplicate outgoing spans/breadcrumbs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
instrumentHttpOutgoingRequestsactually public? It's exported from its module, but not at the top level on the sdk, is it? I think we can remove the unsubscribe logic, since we never use it, and this path is only accessible via instrumentations that are unique by name.