fix(winston-transport): Add attribute serialization #2352
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2352 +/- ##
==========================================
- Coverage 90.97% 90.26% -0.72%
==========================================
Files 146 150 +4
Lines 7492 7167 -325
Branches 1502 1509 +7
==========================================
- Hits 6816 6469 -347
- Misses 676 698 +22
|
|
@trentm we need to do similar thing in other instrumentation like Bunyan and Pino, just wanted to get some feedback about this issue, also it would be good to have serialization code shared between all of these, so maybe this could be added as instrumentation package utils or something, let me know what you think |
|
@hectorhdzg I haven't looked closely yet, but some notes.
|
|
While we're at it, I noticed that the spec says that type any can be:
But the code of Should we also add export type AnyValue = AttributeValue | AnyValueMap;? |
Please note this pr in core which I opened relating to this comment. Looks like the types for |
|
@hectorhdzg I tweaked your patch with this to play with it: diff --git a/packages/winston-transport/src/utils.ts b/packages/winston-transport/src/utils.ts
index e7eddc66..5461ea4c 100644
--- a/packages/winston-transport/src/utils.ts
+++ b/packages/winston-transport/src/utils.ts
@@ -68,7 +68,11 @@ export function emitLogRecord(
const attributes: LogAttributes = {};
for (const key in splat) {
if (Object.prototype.hasOwnProperty.call(splat, key)) {
- attributes[key] = serializeAttribute(splat[key]);
+ if (process.env.USE_HECTOR_PR === 'true') {
+ attributes[key] = serializeAttribute(splat[key]);
+ } else {
+ attributes[key] = splat[key];
+ }
}
}
const logRecord: LogRecord = {Then I ran it with this script. Mostly this is the example from the
const { NodeTracerProvider } = require('@opentelemetry/sdk-trace-node');
const logsAPI = require('@opentelemetry/api-logs');
const {
LoggerProvider,
SimpleLogRecordProcessor,
ConsoleLogRecordExporter,
} = require('@opentelemetry/sdk-logs');
const { WinstonInstrumentation } = require('@opentelemetry/instrumentation-winston');
const { registerInstrumentations } = require('@opentelemetry/instrumentation');
const { OTLPLogExporter } = require('@opentelemetry/exporter-logs-otlp-proto');
const tracerProvider = new NodeTracerProvider();
tracerProvider.register();
const loggerProvider = new LoggerProvider();
var exporter = new ConsoleLogRecordExporter();
// exporter = new OTLPLogExporter();
loggerProvider.addLogRecordProcessor(
new SimpleLogRecordProcessor(exporter)
);
logsAPI.logs.setGlobalLoggerProvider(loggerProvider);
registerInstrumentations({
instrumentations: [ new WinstonInstrumentation() ],
});
const winston = require('winston');
const logger = winston.createLogger({
transports: [new winston.transports.Console()],
})
logger.info('the message', {
aNum: 42,
aStr: 'str',
aUint8Array: new Uint8Array([4,5,6]),
aInt8Array: new Int8Array([-7,8,9]),
anObj: {
numField: 43,
strField: 'str',
uint8ArrayField: new Uint8Array([10,11,12]),
}
});Without your patch: With your patch: I don't think that JSON.stringify() of (I haven't played with an |
|
From the linked issue:
The Any type allows Or perhaps there is a need to guard against things like: logger.info('try to break it', {
os: require('os')
});where some of those attributes are functions, which are clearly not allowed. FWIW, that currently sort of works. When using an OTLP exporter, the receiving OTLP end gets a full `ExportLogsServiceRequest` dump |
I checked what winston logs to console in this case, and looks like it filters out properties which are functions: // logger.js
const winston = require('winston');
// Create a Winston logger instance
const logger = winston.createLogger({
level: 'info',
format: winston.format.simple(),
transports: [
new winston.transports.Console() // Log to the console
],
});
// Example usage of the logger
logger.info('This is an info message', require('os'));
module.exports = logger;Output: Under the hood, it uses the safe-stable-stringify pacakge which contains a switch case where functions are not being handled thus dropped I support dropping these function properties in otlp-transformations, but this should probably not be a blocker for this PR which is about recording them into attributes correctly. we can open an issue and work on this separately now or later. |
|
@trentm @blumamir maybe we can just reuse toAttributes and toAnyValue methods included in otlp-transformer, I was not aware of these, maybe moving these to core package and all Log Instrumentations can reuse it, does that sound good to you? we will not use JSON.stringify in objects, because we will have Map<string, any> instead, we can JSON stringify as needed when exporting. |
I don't yet see the harm in allowing whatever values through and letting the OTLP serializer handle it. I do see some potential harm in fully validating that log record attributes are a valid type: It could ahve a significant perf impact. Because arrays and maps are allowed, it would require recursing through the full object tree and checking every key and array element's type. That full object walk would then be done again in the serializer/otlp-transformer. On the SIG call, Dan mentioned considering a depth limit in the otlp-transformer. We also briefly talked about a circular-ref check in the transformer -- though that is a chunk of work. @hectorhdzg Are there specific issues that you were hitting in Log Record usage that you are able to post so we can weight options? |
|
@trentm originally this issue came with JS Errors not being able to serialize with JSON.stringify and causing the attribute to be not useful in our Azure Exporter, there were some internal discussions about which part of the code is responsible for ensuring the attributes are correct types, like dictionaries instead of any generic object, objects with circular dependencies, etc. As this is mentioned in OTel spec I was trying to ensure the attributes are correct, in our case we have tons of customers not using otlp-transformer so they don't get this serialization out of the box, we can add serialization in our side, we already have some though, but just wondering which is the correct place to have that so everyone get the advantage of it. |
Oh, so an export mechanism other that OTLP is typically being used? Can you provide details on that exporter? (Not necessarily needed here, though, I'm mostly curious.)
Any specifics on that would be helpful -- though is possibly beside the point. In this mostly-unrelated package (that I've worked on) that does formatting of Winston output to "ECS logging" format (a structured format that dictates some of the fields) here is a long comment describing the various scenarios I'd hit with a Winston logger and I wonder if the case of handing JS Errors could, at least partially, be handled separate from some general solution that needs to walk full object trees.
Dictionaries vs generic objects isn't a thing that I think we'll want to distinguish for JS. It is a subjective distinction in JS.
Circular refs: Yes, this one is interesting to discuss. Dan suggested punting the responsibility back to the user for this on the last JS SIG call. I'm a little less sure. The main Node.js logging libs have settled on handling circular refs gracefully for the user -- mostly via the
Other cases to perhaps consider:
|
Which problem is this PR solving?
Fixes #2351
Short description of the changes
Added serialization of Winston parameters to ensure supported types are passed through.