fix(nodejs): bundle missing winston-transport dependency in Lambda layer - #2517
Open
pujitha24 wants to merge 1 commit into
Open
fix(nodejs): bundle missing winston-transport dependency in Lambda layer#2517pujitha24 wants to merge 1 commit into
pujitha24 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>
Author
|
This has been rebased and green for a little while now - happy to make any changes if something would help move review along. |
Member
|
What are winston logs? I've never heard of this before. Please provide a bit more context. |
Author
|
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 ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While looking at the Winston issue, I found that the Lambda layer already includes
@opentelemetry/instrumentation-winston, but the transport it needs for exporting logs isn't actually bundled.Webpack was already warning that it couldn't resolve
@opentelemetry/winston-transport. That require happens when the Winston instrumentation tries to attach its OTel transport, so it fails silently and log export never gets enabled. Trace/span ID injection is a separate path and is not affected.This adds
@opentelemetry/winston-transportto the layer dependencies and adds a test using Winston itself to verify the OTel transport gets attached.I also confirmed the webpack warning disappears and the layer build, tests and lint pass.
AI assistance: I used AI tools while investigating this and preparing the change; I have reviewed and tested it myself.
Fixes #2065