feat(instrumentation-console): Add console intrumentation#3445
feat(instrumentation-console): Add console intrumentation#3445hectorhdzg wants to merge 4 commits intoopen-telemetry:mainfrom
Conversation
| }); | ||
|
|
||
| it('emits LogRecord for console.trace', () => { | ||
| console.trace('trace message'); |
There was a problem hiding this comment.
Looks like we're ensuring that console.trace() contains the message field here, but trace should also print a stack trace. We should ensure that the stack is also captured with this instrumentation.
JacksonWeber
left a comment
There was a problem hiding this comment.
Minor comment, but overall LGTM.
|
Might be worth syncing with the browser SIG, they also currently have a PR open, but they'll likely integrate it into their single instrumentation package: |
|
@pichlermarc this is specific to Node.js console not browser, we should ensure there are no name conflicts and customer confusion, I can rename this package to @opentelemetry/instrumentation-console-nodejs or something like that, what you think? |
| this._originals = new Map(); | ||
| } | ||
| for (const method of Object.keys(CONSOLE_METHODS)) { | ||
| const original = (console as any)[method]; |
There was a problem hiding this comment.
This is not using require/import in the middle, that would work if people import manually the console from their code, that is usually not the case and people rely on global console.
+1. FWIW, we've used "-node" suffix in a couple cases: So perhaps |
|
759b2a3 to
f25d731
Compare
Adding console instrumentation, following similar configurations and functionality in other Log instrumentations in this project like Bunyan, Winston and Pino
#3444