fix(nodejs): bundle missing winston-transport dependency in Lambda layer - #2517
fix(nodejs): bundle missing winston-transport dependency in Lambda layer#2517pujitha24 wants to merge 1 commit into
Conversation
Motivation: Enabling `winston` via OTEL_NODE_ENABLED_INSTRUMENTATIONS never exported winston logs to the configured OTLP backend. The layer's webpack build already emitted "Module not found: Error: Can't resolve '@opentelemetry/winston-transport'" for the require inside @opentelemetry/instrumentation-winston's patched `configure()`, which attaches an OpenTelemetryTransportV3 transport to export logs. Since @opentelemetry/winston-transport was declared as neither a dependency of @opentelemetry/instrumentation-winston nor of this layer's package.json, it was never bundled, the require always threw MODULE_NOT_FOUND inside a caught try/catch, and the transport (and therefore log export) was silently skipped. Log correlation (trace_id/span_id injection into log records via the patched write/log methods) is a separate code path and is unaffected by this bug. This does not confirm the race condition theorized in the report (async LoggerProvider creation racing synchronous instrumentation registration in wrapper.ts) - init.mjs awaits both wrapper.init() and wrapper.wrap() to completion, including LoggerProvider creation, before the Lambda handler module is ever loaded, so the LoggerProvider is already set by the time user code creates a winston logger. Approach: Add `@opentelemetry/winston-transport` to the layer's dependencies so it is bundled by webpack alongside the other auto-instrumentation packages. Add `winston` as a devDependency to exercise the real auto-instrumentation path in a new regression test. Validation: - `npm run compile:webpack` in nodejs/packages/layer: before this change, printed a "Module not found" warning for '@opentelemetry/winston-transport'; after, compiles cleanly with winston-transport bundled. - `npm run build` in nodejs/packages/layer: full build (webpack, externals install, packaging) succeeds and produces layer.zip. - Added nodejs/packages/layer/test/winston-instrumentation.spec.ts, which enables WinstonInstrumentation, requires winston, creates a logger, and asserts an OpenTelemetryTransportV3 transport gets attached. Verified this test fails with an assertion error when @opentelemetry/winston-transport is removed from node_modules (reproducing the reported symptom), and passes with the dependency present. - `npm test` in nodejs/packages/layer (test:cjs + test:esm): 16/16 passing, no regressions in existing wrapper/handler tests. - `npm run lint` in nodejs/packages/layer: clean. Report: open-telemetry#2065 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
This has been rebased and green for a little while now - happy to make any changes if something would help move review along. |
|
What are winston logs? I've never heard of this before. Please provide a bit more context. |
|
Sorry, should've defined that upfront. Winston (https://github.com/winstonjs/winston) is a popular Node.js logging library. This layer already ships auto-instrumentation for it ( |
Motivation:
winston is a widely-used Node.js logging
library. This SDK layer already ships auto-instrumentation for it
(
@opentelemetry/instrumentation-winston, enabled viaOTEL_NODE_ENABLED_INSTRUMENTATIONS) that is supposed to attach an OTLP-exportingtransport to a user's winston logger so their winston log calls get exported as
OTel log records, in addition to injecting trace_id/span_id into them.
The log-export half of that was broken: the layer's webpack build already emitted
"Module not found: Error: Can't resolve '@opentelemetry/winston-transport'" for
the require inside instrumentation-winston's patched
configure(), which is whatattaches the OpenTelemetryTransportV3 transport. Since
@opentelemetry/winston-transportwas declared as neither a dependency of@opentelemetry/instrumentation-winstonnor of this layer's package.json, it wasnever bundled, the require always threw MODULE_NOT_FOUND inside a caught
try/catch, and the transport (and therefore log export) was silently skipped.
Log correlation (trace_id/span_id injection into log records via the patched
write/log methods) is a separate code path and is unaffected by this bug.
This does not confirm the race condition theorized in the report (async
LoggerProvider creation racing synchronous instrumentation registration in
wrapper.ts) - init.mjs awaits both wrapper.init() and wrapper.wrap() to
completion, including LoggerProvider creation, before the Lambda handler module
is ever loaded, so the LoggerProvider is already set by the time user code
creates a winston logger.
Approach:
Add
@opentelemetry/winston-transportto the layer's dependencies so it isbundled by webpack alongside the other auto-instrumentation packages. Add
winstonas a devDependency to exercise the real auto-instrumentation path in anew regression test.
Validation:
npm run compile:webpackin nodejs/packages/layer: before this change,printed a "Module not found" warning for '@opentelemetry/winston-transport';
after, compiles cleanly with winston-transport bundled.
npm run buildin nodejs/packages/layer: full build (webpack, externalsinstall, packaging) succeeds and produces layer.zip.
enables WinstonInstrumentation, requires winston, creates a logger, and
asserts an OpenTelemetryTransportV3 transport gets attached. Verified this
test fails with an assertion error when @opentelemetry/winston-transport
is removed from node_modules (reproducing the reported symptom), and
passes with the dependency present.
npm testin nodejs/packages/layer (test:cjs + test:esm): 16/16 passing,no regressions in existing wrapper/handler tests.
npm run lintin nodejs/packages/layer: clean.Report: #2065
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com
Fixes #2065