Skip to content
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

[winston-transport] - Transform instances of Error so we don't "lose" them #2443

Open
mderriey opened this issue Sep 23, 2024 · 0 comments
Open

Comments

@mderriey
Copy link

From open-telemetry/opentelemetry-js#4828.

Is your feature request related to a problem? Please describe

We often log errors with winston, like:

try {
  // Code that throws
} catch (e) {
  logger.error('Unhandled error', {
    error: e
  })
}

Error instances are not valid attributes for log records, and they're not transformed by the winston transport, so we get:

  1. A console warning, "Invalid attribute value set for key: error".
  2. And most importantly, most of the time we don't get the error details in the resulting telemetry.

Describe the solution you'd like to see

I'd love it if the winston transport could apply some heuristics to make sure the attributes for the log records are valid values.

As a note, JSON.stringifying objects may not be sufficient since many properties from Error are not enumerable.

> JSON.stringify(new Error('Boom'))
'{}'

I've noticed that the Azure Monitor exporter has a trick to get more info from an Error instance:

> const error = new Error('Boom')
undefined
> JSON.stringify(error, Object.getOwnPropertyNames(error))
'{"stack":"Error: Boom\\n    at REPL11:1:15\\n    at ContextifyScript.runInThisContext (node:vm:136:12)\\n    at REPLServer.defaultEval (node:repl:598:22)\\n    at bound (node:domain:432:15)\\n    at REPLServer.runBound [as eval] (node:domain:443:12)\\n    at REPLServer.onLine (node:repl:927:10)\\n    at REPLServer.emit (node:events:531:35)\\n    at REPLServer.emit (node:domain:488:12)\\n    at [_onLine] [as _onLine] (node:internal/readline/interface:417:12)\\n    at [_line] [as _line] (node:internal/readline/interface:888:18)","message":"Boom"}'

This is better, however I'm not sure whether it takes all cases into account.

A quick look on npm seems to suggest that the serialize-error package is the most comprehensive way to get all information one needs about an Error. The main issue seems to be that it's an ESM-only package, and even if it wasn't, I'm not sure whether you'd be open to take a dependency on it. Maybe we could vendor the subset of the code in this package?

Describe alternatives you've considered

At this stage, none, it feels like this transport is the "point of entry" into the OTel world, so it should ideally respect the OTel contract when it comes to attributes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant