feat(core): pass client logger to request handlers - #2170
Conversation
TypeScript strict mode cannot narrow `value` from the full `NodeHttpHandlerOptions[typeof key]` union when branching on `key === "logger"`. Add an explicit cast to fix the build:types target in CI.
kuhe
left a comment
There was a problem hiding this comment.
look at additional requestHandler implementations in AWS SDK JS.
at this time, handlers which resolve config asynchronously don't offer a way to query whether there is a higher priority logger set on the requestHandler at requestHandler initialization.
We need a plan to account for this problem.
| runtimeConfig: HttpHandlerExtensionConfigType<HandlerConfig> | ||
| ) => { | ||
| if ((runtimeConfig as any).logger) { | ||
| runtimeConfig.httpHandler?.updateHttpClientConfig("logger" as keyof HandlerConfig, (runtimeConfig as any).logger); |
There was a problem hiding this comment.
This is an unconditional overwrite of the handler logger with the client logger, which is not always intended.
Here are the design requirements:
H = handler, C = client
- if H has no explicit logger and C has a logger (default or otherwise), provide C logger to H logger
- in other words, if H has an explicit logger, do not overwrite it
- if H resolves its logger asynchronously (NodeHttpHandler), it must still accept the C logger synchronously and save it for later use.
- the system must work with all 5 team-authored hanlders: Fetch, Node, Undici, Xhr, and WebsocketFetch
- no handler level method should be added, such as
setFallbackLogger(Logger).
We may want to customize updateHttpClientConfig implementations in all 5 handlers such that receiving a special key sets the fallback logger, but do not define this key on the public input config of the handler itself.
Example:
updateHttpClientConfig(key: symbol, value: unknown): void;
updateHttpClientConfig(key: keyof XhrHttpHandlerOptions, value: XhrHttpHandlerOptions[typeof key]): void;There was a problem hiding this comment.
The client now passes its logger to the request handler through updateHttpClientConfig, under a new FALLBACK_LOGGER key exported from @smithy/core/protocols
node-http-handler uses it for its socket exhaustion and request timeout warnings when it has no logger of its own, replacing config.logger ?? console with config.logger ?? fallbackLogger
There was a problem hiding this comment.
found that this never actually runs, the client offers its logger but updateHttpClientConfig call is skipped. I think it's because getHttpHandlerExtensionConfig expects the handler at runtimeConfig.httpHandler but clients populate requestHandler instead. And since httpHandler is never set, the guard httpHandler?. on updateHttpClientConfig skips the call entirely. Should it move to a point that actually reads requestHandler?
There was a problem hiding this comment.
getHttpHandlerExtensionConfig and resolveHttpHandlerRuntimeConfig now use requestHandler (the field clients actually set) instead of httpHandler (which was always undefined)
This was already broken where runtimeConfig.httpHandler! non-null assertion has been silently reading undefined since it was introduced. Nothing surfaced it because nothing called these methods until now.
Both HttpHandlerExtensionConfigType and resolveHttpHandlerRuntimeConfig return type change shape but both are @internal. Noted in the changeset.
| value: FetchHttpHandlerOptions[keyof FetchHttpHandlerOptions] | Logger | ||
| ): void { | ||
| if (key === FALLBACK_LOGGER) { | ||
| this.fallbackLogger = value as Logger; |
There was a problem hiding this comment.
fallbackLogger gets set here but isn't referenced anywhere else, same for undici. Only node-http-handler consumes it.
| /** | ||
| * Client logger, used only when this handler has no logger of its own. | ||
| */ | ||
| private fallbackLogger?: Logger; |
There was a problem hiding this comment.
don't use a separate field. the logger can be assigned if receiving a fallback logger and no logger exists
There was a problem hiding this comment.
fallbackLogger field removed, now assigns directly into config.logger.
| * | ||
| * @internal | ||
| */ | ||
| export const FALLBACK_LOGGER: unique symbol = Symbol.for("smithy.httpHandler.fallbackLogger"); |
There was a problem hiding this comment.
Symbol.for is the opposite of unique. Remove the unique type keyword here.
There was a problem hiding this comment.
done, now Symbol.for("logger") with no unique.
| * | ||
| * The key may also be {@link FALLBACK_LOGGER}, with which a client offers its | ||
| * logger for use only when the handler has no logger of its own. Handlers | ||
| * that predate that key ignore it. |
There was a problem hiding this comment.
description goes first, above the @internal annotation
| * that predate that key ignore it. | ||
| */ | ||
| updateHttpClientConfig(key: keyof HttpHandlerConfig, value: HttpHandlerConfig[typeof key]): void; | ||
| updateHttpClientConfig( |
There was a problem hiding this comment.
don't change the signature of this method, I believe this could be breaking for consuming implementers.
There was a problem hiding this comment.
reverted. The fallback logger key is now handled at each handler's call site.
| export { Field } from "./protocol-http/Field"; | ||
| export { Fields, type FieldsOptions } from "./protocol-http/Fields"; | ||
| export { type HttpHandler, type HttpHandlerUserInput } from "./protocol-http/httpHandler"; | ||
| export { FALLBACK_LOGGER } from "./protocol-http/fallbackLogger"; |
There was a problem hiding this comment.
let's not export this, Symbol.for("string") can be equality-compared across contexts.
The value can be Symbol.for("logger"), since being a symbol is already differentiated from "logger" as a normal string.
There was a problem hiding this comment.
no longer exported. Each handler declares its own Symbol.for("logger") now.
| * | ||
| * @internal | ||
| */ | ||
| const FALLBACK_LOGGER: symbol = Symbol.for("logger"); |
There was a problem hiding this comment.
inlined in both handlers.
There was a problem hiding this comment.
Also inlined it at the offer site in httpExtensionConfig.ts which let me drop fallbackLogger.ts entirely since it only existed to export that one.
There was a problem hiding this comment.
recap for reviewers:
passes the client's logger to request handlers as a fallback. Handlers use it for socket exhaustion warnings and timeout messages only if they don't already have their own logger. When the client logger is a NoOpLogger, the call is skipped entirely, so handlers keep their console defaults when the client hasn't configured logging.
Also fixes a latent bug in httpExtensionConfig.ts where getHttpHandlerExtensionConfig and resolveHttpHandlerRuntimeConfig were reading/writing runtimeConfig.httpHandler which clients never populate. Changed to requestHandler. Both affected types are @internal.
FetchHttpHandlerOptions has no logger field and the handler doesn't log anything. So fetch-http-handler is unchanged. If we want consistency across, I can add a logger field to FetchHttpHandlerOptions as a public option.
Issue #, if available:
aws/aws-sdk-js-v3#6130
JS-7027
Description of changes:
Passes the client's logger to request handlers as a fallback. Handlers use it for socket exhaustion warnings and timeout messages only if they don't already have their own logger. When the client logger is a
NoOpLogger, the call is skipped entirely, so handlers keep their console defaults when the client hasn't configured logging.Also fixes a latent bug in
httpExtensionConfig.tswheregetHttpHandlerExtensionConfigandresolveHttpHandlerRuntimeConfigwere reading/writingruntimeConfig.httpHandlerwhich clients never populate. Changed torequestHandler. Both affected types are@internal.Testing
getHttpHandlerExtensionConfigNodeHttpHandler.updateHttpClientConfignullishUndiciHttpHandler.updateHttpClientConfigBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.