-
Notifications
You must be signed in to change notification settings - Fork 621
fix(winston-transport, instrumentation-winston): get the correct severityText and severityNumber with winston.format.colorize() is used #2956
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
Conversation
…rityText and severityNumber with winston.format.colorize() is used
Before this change the "log sending" feature of instr-winston would
incorrectly include ANSI color codes in the `severityText` for emitted
OTel LogRecords when the winston logger being instrumented was
configured with the `colorize()` formatter, e.g.:
const logger = winston.createLogger({
format: winston.format.combine(
winston.format.timestamp(),
winston.format.colorize(),
winston.format.simple()
),
...
This is because the winston-transport.OpenTelemetryTransportV3 was using
the Winston log info's "level" attribute... which was tweaked by
`colorize()`. Winston log `info` objects have a `Symbol.for("info")`
property which has the unchanged level name:
https://github.com/winstonjs/winston#streams-objectmode-and-info-objects
This commit *also* changes (fix) what `npm test` runs in the
instrumentation-winston package. Before this, `npm test` was attempting
to test against winston@2 and winston@3 -- presumably for better
coverage. However, it ended up only ever testing with winston@2
(`test-v1-v2` would install winston@2 and test, then `test-v3` would
run the same test again with whatever current version is installed,
which is now winston@2).
The (IMO) correct answer here is:
- `npm test` tests against the latest ver of the target module (winston@3)
- `npm run test-all-versions` handles testing against other versions
- coverage can take a back seat. (Aside: changes coming as part of
open-telemetry#2866
will, IIUC, provide coverage from test-all-versions runs.)
Yup. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2956 +/- ##
=======================================
Coverage ? 89.90%
=======================================
Files ? 188
Lines ? 9222
Branches ? 1900
=======================================
Hits ? 8291
Misses ? 931
Partials ? 0
🚀 New features to boost your workflow:
|
hectorhdzg
left a comment
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.
LGTM, thanks for taking care of this
Co-authored-by: Hector Hernandez <[email protected]>
…n the info object passed to OpenTelemetryTransportV3 for testing
Before this change the "log sending" feature of instr-winston would
incorrectly include ANSI color codes in the
severityTextfor emittedOTel LogRecords when the winston logger being instrumented was
configured with the
colorize()formatter, e.g.:This is because the winston-transport.OpenTelemetryTransportV3 was using
the Winston log info's "level" attribute... which was tweaked by
colorize(). Winston loginfoobjects have aSymbol.for("info")property which has the unchanged level name:
https://github.com/winstonjs/winston#streams-objectmode-and-info-objects
This commit also changes (fix) what
npm testruns in theinstrumentation-winston package. Before this,
npm testwas attemptingto test against winston@2 and winston@3 -- presumably for better
coverage. However, it ended up only ever testing with winston@2
(
test-v1-v2would install winston@2 and test, thentest-v3wouldrun the same test again with whatever current version is installed,
which is now winston@2).
The (IMO) correct answer here is:
npm testtests against the latest ver of the target module (winston@3)npm run test-all-versionshandles testing against other versionschore: improve test workflow and coverage reports #2866
will, IIUC, provide coverage from test-all-versions runs.)
Closes: #2952