Skip to content

[SER-281] Log Errors#5

Merged
rmharrison merged 5 commits into
developfrom
bug/SER-281-log-errors
Apr 16, 2019
Merged

[SER-281] Log Errors#5
rmharrison merged 5 commits into
developfrom
bug/SER-281-log-errors

Conversation

@mountHouli
Copy link
Copy Markdown

@mountHouli mountHouli commented Apr 9, 2019

Jira Ticket(s)

SER-281

To Test

Test 1

yarn test.

Test 2 (optional):

Step A: Setup logging

In format.test.js, in each of the beforeEach() callbacks that specify a jest mock function (e.g. spy = jest.fn()), add this logging statement: console.log(expectedMessage);

Step B: Setup script

In package.json, add this script "test-with-logging": "yarn install && jest --verbose=false",

Step C: Run it

yarn test-with-logging

Results: Logged to your terminal should be each of the desired outcomes listed in the jira ticket.

@mountHouli mountHouli changed the title [SER-281] Entire Implementation [SER-281] Log Errors Apr 9, 2019
Copy link
Copy Markdown

@orndorffgrant orndorffgrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very 🔥

Copy link
Copy Markdown
Contributor

@rmharrison rmharrison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR

The function signatures don't work quite how I expected. Some of this is bunyan vs winston signature standards.

Most are edge-cases that behave unexpectedly.

  • Which are behaving as you expect?
  • Which, if any, should be supported?

And thank you for explicitly enumerating all of the test cases!

In 20+ test cases, the only enumerated test case that doesn't behave as I expected is (B), the concatenation of LoggerMsg and ErrorMsg with logger.error("LoggerMsg", new Error("ErrorMsg"))

FFR: To reproduce, copy/paste the lines from the README.md:L13-32 and use the logger.error signatures enumerated below.

(A) Resolved: I was surprised that the err first signature didn't work (this is a difference with bunyan)

logger.error(new Error('abc'), 'msg')
{
  "service": "test-service",
  "logger": "Winston-JSON-Formatter",
  "hostname": "...",
  "level": "error",
  "msg": {},
  "meta": {
    "service": {
      "version": "1.0.0",
      "node_env": ""
    },
    "logger": {
      "time": "2019-04-09T23:00:31.789Z"
    },
    "event": {}
  }
}
  • No err, but this is expected from the winston standard.

Thank you for linking the specific winston fix PR in your JIRA issue: winstonjs/winston#1562 (comment)
Here, they enumerate the supported signatures, which does not include err first.

(B) Unresolved: LoggerMsg and ErrorMsg behaviour

Unexpected...

> logger.error('LoggerMsg', new Error('ErrorMsg'))
{
  "service": "test-service",
  "logger": "Winston-JSON-Formatter",
  "hostname": "...",
  "level": "error",
  "msg": "LoggerMsgErrorMsg", # <~~~ Concatated LoggerMsg and ErrorMsg
  "meta": {
    "service": {
      "version": "1.0.0",
      "node_env": ""
    },
    "logger": {
      "time": "2019-04-09T23:04:01.682Z"
    },
    "event": {}
  },
  "err": {
    "stack": "..." # <~~~ err.name doesn't appear
  }
}
  • LoggerMsg and ErrorMsg are concatinated
  • err.name doesn't appear

Expected...

> logger.error(new Error('ErrorMsg'))
{
  "service": "test-service",
  "logger": "Winston-JSON-Formatter",
  "hostname": "...",
  "level": "error",
  "msg": "ErrorMsg",
  "meta": {
    "service": {
      "version": "1.0.0",
      "node_env": ""
    },
    "logger": {
      "time": "2019-04-09T23:05:30.698Z"
    },
    "event": {}
  },
  "err": {
    "name": "Error",
    "stack": "..."
  }
}

err.name appears as expected.

(C) Unresolved: Unsupported signature: Err, {}

> logger.error(new Error('ErrorMsg'), {a: 'a', b: 'b'})
{
  "service": "test-service",
  "logger": "Winston-JSON-Formatter",
  "hostname": "...",
  "level": "error",
  "msg": {},
  "meta": {
    "service": {
      "version": "1.0.0",
      "node_env": ""
    },
    "logger": {
      "time": "2019-04-09T23:11:57.836Z"
    },
    "event": {
      "a": "a",
      "b": "b"
    }
  }
}
  • No err

(D) Unresolved: Unsupported signature: FunctionMsg, Err, {}

> logger.error('LoggerMsg', new Error('ErrorMsg'), {a: 'a', b: 'b'})
{
  "service": "test-service",
  "logger": "Winston-JSON-Formatter",
  "hostname": "Hargrove",
  "level": "error",
  "msg": "LoggerMsgErrorMsg",
  "meta": {
    "service": {
      "version": "1.0.0",
      "node_env": ""
    },
    "logger": {
      "time": "2019-04-09T23:12:57.049Z"
    },
    "event": {} # <~~~ {a: 'a', b: 'b'} don't appear here
  },
  "err": {
    "stack": "..."
  }
}

Clarification

Currently, code is not "magic" and appears under meta.event
If code is part of an Error object, should it appear under err.code?

Comment thread src/index.js
Comment thread src/index.js
@rmharrison
Copy link
Copy Markdown
Contributor

Please update readme:

  • Update JSON log format
  • New screenshot for console logger

@mountHouli
Copy link
Copy Markdown
Author

Regarding item (B)

The concatenated messages are a winston bug. See winstonjs/winston#1634

The lack of err.name / info.name is a winston bug. See winstonjs/winston#1635

Regarding item (C)

Fixed. See commit a8ed890

Regarding item (D)

This case is not supported by winston. See examples in winstonjs/winston#1562

@mountHouli mountHouli force-pushed the bug/SER-281-log-errors branch from ec5ffa4 to 08b1198 Compare April 12, 2019 02:28
@mountHouli mountHouli force-pushed the bug/SER-281-log-errors branch from 08b1198 to a911604 Compare April 12, 2019 02:32
@rmharrison rmharrison merged commit b4f76f9 into develop Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants