feat: add @opentelemetry/instrumentation-console package for capturing console calls as OpenTelemetry logs#98
Conversation
…ing console method calls as OpenTelemetry logs
|
|
Co-authored-by: Jared Freeze <overbalance@users.noreply.github.com>
|
@overbalance happy new year. I got back at this PR and fixed the linting error. May you reapprove the workflow please? |
|
@drdreo happy new year! the license header rules changed on main. to repair please update from main and run |
|
and |
overbalance
left a comment
overbalance
left a comment
There was a problem hiding this comment.
Code looks good. Let me know if you have concerns about my suggestion.
One issue I ran into was an infinite loop if I use the recommended OTel example https://opentelemetry.io/docs/languages/js/getting-started/browser/#creating-an-exporter
Would you mind adding a section to the docs stating the conflict if you also use ConsoleSpanExporter during development?
|
Hej, i've addressed your feedback. I added a simple vite test app at |
Co-authored-by: Jared Freeze <overbalance@users.noreply.github.com>
|
@martinkuba adressed your feedback. Thanks for the input. |
|
@drdreo We discussed the meta tag use case for passing trace context from the backend in last week's SIG meeting, and we decided to not include it for now. I have created this this issue #129 to discuss further. Can you please remove that part for now? That way we can merge this PR, and if we decide we want to include it later, we can add separately. |
overbalance
left a comment
overbalance
left a comment
There was a problem hiding this comment.
@drdreo There was an issue in another instrumentation recently where repeatedly calling enable() would add listeners over and over. I think this code may be subject to that as well. Does this also need a guard?
|
@overbalance good point. I can add something like this, since the override enable(): void {
if (this.isEnabled()) {
return;
} |
There was a problem hiding this comment.
Overall looks good. #175 moved all instrumentations in a single package I think this one should live there too.
Checklist
- move the code into
/packages/instrumentation/src/console - set the scope name to
@opentelemetrry/browser-instrumentation/console
| const methods = this._getLogMethods(); | ||
| for (const method of methods) { | ||
| if (typeof console[method] === 'function') { | ||
| this._wrap(console, method, this._patchConsoleMethod(method)); |
There was a problem hiding this comment.
nit: Although it's unlikely that another instrumentation patches console methods there was a discussion recently since there is a potential incompatibility between instrumentations when they unpatch APIs.
The conclusion was that instrumentations should patch the API once and the patched functions should check for the enabled state of the instrumentation. ref: #204 (comment)
There was a problem hiding this comment.
I have moved the console instrumentation to the new folder structure and read through the unpatching discussion. So no unwrap on disable anymore, just is active false
There was a problem hiding this comment.
@drdreo thanks for doing it :)
I'd leverage this conversation to point to another topic/question which is if the configuration should be static or could change in time. In this instrumentation I see the console methods option is final and if the user excluded a method there is no way to change that unless the config is changed and the application reloaded.
IMHO if a instrumentation can observe changes in configuration and behave accordingly the user has more control on the telemetry generated and even can implement mechanisms to tune/config instrumentations without the need of a reload or re-deploy. A scenario that comes to mind is an app containing a bug which spits a ton of errors in the console (like in an endless loop). If the app is accessed my many users this error rate (logs sent to OTLP endpoint) could be multiplied by the # of users. In that case I wish I had a way to tell the instrumented apps to stop sending logs, at least of error severity.
There was a problem hiding this comment.
Worth mention with "dynamic config" we should have only one instance of the instrumentation for all the test and just adjust the configuration for the feature we want to test. I think having duplicated instances of the same instrumentation in the same web page is not desired.
There was a problem hiding this comment.
Hmm, i think we could easily add that to dynamically check the configured methods on call and just patch all from the start.
Want me to add it here or create a new feature PR?
There was a problem hiding this comment.
would you consider doing it for this PR? I'm okay if we do it in a follow up PR
There was a problem hiding this comment.
@david-luna i have added dynamic config for log methods in this PR :)
overbalance
left a comment
overbalance
left a comment
There was a problem hiding this comment.
Oops. I missed what folder this was in.
…ntation # Conflicts: # package-lock.json
|
@drdreo thank you for your contribution :) |
@david-luna Thanks for having me :) glad to contribute |
|
@drdreo Thanks again, this has been released. It would be great to add an entry to the readme inside the instrumentation folder. |
I have created #260, which is related |
5 participants
Which problem is this PR solving?
Users should be able to capture console logs and export them to OpenTelemetry. This PR adds a browser console method instrumentation which sends them as opentelemetry logs
Fixes #15
Type of change
How Has This Been Tested?
Unit tests for console logs being sent to the in memory processor as well as the config options were verified in unit tests with edge case testing for the custom serializer failing.
Checklist:
OPEN QUESTIONS
what semantic attributes?
severity numberfor log vs info?
is the traceparent of the current active span enough?