fix(winston): do not remove info.level field (#174)

Winston's logform docs state that 'info' objects should always
have 'level' and 'message' fields. Some downstream transforms
and transports can rely on this. Instead the 'ecsStringify' will
handle excluding the 'level' field from the stringification.

Closes: #173

Author: trentm

docs/winston.asciidoc

@@ -339,11 +339,22 @@ const logger = winston.createLogger({
 ** `eventDataset` +{type-string}+ A "event.dataset" value. If specified this overrides the default of using `${serviceVersion}`.
 
 Create a formatter for winston that converts fields on the log record info
-objecct to ECS Logging format.
+object to ECS Logging format.
 
 [float]
 [[winston-ref-ecsStringify]]
 ==== `ecsStringify([options])`
 
 Create a formatter for winston that stringifies/serializes the log record to
-JSON. (This is very similar to `logform.json()`.)
+JSON.
+
+This is similar to `logform.json()`. They both use the `safe-stable-stringify`
+package to produce the JSON. Some differences:
+
+* This stringifier skips serializing the `level` field, because it is not
+  an ECS field.
+* Winston provides a `replacer` that converts bigints to strings The argument
+  *for* doing so is that a *JavaScript* JSON parser looses precision when
+  parsing a bigint. The argument against is that a BigInt changes type to a
+  string rather than a number. For now this stringifier does not convert
+  BitInts to strings.

packages/ecs-winston-format/CHANGELOG.md

@@ -1,5 +1,12 @@
 # @elastic/ecs-winston-format Changelog
 
+## v1.5.2
+
+- Fix the Winston transformers to *not* delete the `level` property from the
+  `info` object, because https://github.com/winstonjs/logform#info-objects says
+  "Every `info` must have at least the `level` and `message` properties".
+  (https://github.com/elastic/ecs-logging-nodejs/issues/173)
+
 ## v1.5.0
 
 - Add `ecsFields` and `ecsStringify` exports that are winston formatters

packages/ecs-winston-format/index.js

@@ -35,16 +35,6 @@ try {
   // Silently ignore.
 }
 
-// Comparison of Winston's `logform.json()` for JSON serialization and
-// `ecsStringify()`.
-// - They both use `safe-stable-stringify`.
-// - Winston's exposes its `safe-stable-stringify` options, but doesn't document
-//   this. (https://github.com/winstonjs/logform#json)
-// - Winston provides a `replacer` that converts bigints to strings. Doing
-//   that is debatable. The argument *for* is that a *JavaScript* JSON parser
-//   looses precision when parsing a bigint. The argument against is that a
-//   BigInt changes type to a string rather than a number.
-// TODO: These differences should make it to docs somewhere.
 const stringify = safeStableStringify.configure()
 
 /**
@@ -183,13 +173,10 @@ class EcsFieldsTransform {
     // Core ECS logging fields.
     info['@timestamp'] = new Date().toISOString()
     info['log.level'] = info.level
-    // Removing 'level' might cause trouble for downstream winston formatters
-    // given that https://github.com/winstonjs/logform#info-objects says:
-    //
-    // > Every info must have at least the level and message properties:
-    //
-    // However info still has a `info[Symbol.for('level')]` for more reliable use.
-    delete info.level
+    // Note: We do *not* remove `info.level`, even though it is not an ECS
+    // field, because https://github.com/winstonjs/logform#info-objects says:
+    // "Every info must have at least the level and message properties".
+    // Instead, it will be excluded from serialization in `EcsStringifyTransform`.
     info['ecs.version'] = version
 
     let apm = null
@@ -290,7 +277,11 @@ class EcsStringifyTransform {
   }
 
   transform (info, opts) {
-    info[MESSAGE] = stringify(info)
+    // `info.level` must stay (see note above), but we don't want to serialize
+    // it, so exclude it from the stringified fields. There *is* a perf cost
+    // for this.
+    const { level, ...infoSansLevel } = info
+    info[MESSAGE] = stringify(infoSansLevel)
     return info
   }
 }

packages/ecs-winston-format/test/basic.test.js

@@ -85,6 +85,38 @@ test('Should set expected @timestamp, "log.level", message', t => {
   t.end()
 })
 
+// Per https://github.com/winstonjs/logform#info-objects
+// "Every info must have at least the level and message properties"
+test('Log record "info" objects should still have info.level and info.message', t => {
+  class CheckInfoTransform {
+    constructor (opts) {
+      this.options = opts
+    }
+
+    transform (info, _opts) {
+      t.ok(info.message, 'info.message exists')
+      t.ok(info.level, 'info.level exists')
+      return info
+    }
+  }
+
+  const cap = new CaptureTransport()
+  const logger = winston.createLogger({
+    format: winston.format.combine(
+      ecsFormat(),
+      new CheckInfoTransform()
+    ),
+    transports: [cap]
+  })
+
+  logger.info('hi')
+  // Should *not* have a 'level' field.
+  const rec = cap.records[0]
+  t.notOk(rec.level, 'should not have a "level" field')
+
+  t.end()
+})
+
 test('Should append additional fields to the log record', t => {
   const cap = new CaptureTransport()
   const logger = winston.createLogger({