-
Notifications
You must be signed in to change notification settings - Fork 532
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
feat(instrumentation-bunyan): add log sending to Logs Bridge API #1713
feat(instrumentation-bunyan): add log sending to Logs Bridge API #1713
Conversation
This extends the Bunyan instrumentation to automatically add a Bunyan stream to created loggers that will send log records to the Logs Bridge API: https://opentelemetry.io/docs/specs/otel/logs/bridge-api/ Now that the instrumentation supports separate "injection" of fields and "bridging" of log records functionality, this also adds two boolean options to disable those independently: `enableInjection` and `enableLogsBridge`. This also updates the instrumentation to work with ES module usage. Closes: open-telemetry#1559
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1713 +/- ##
==========================================
+ Coverage 91.42% 91.50% +0.07%
==========================================
Files 143 144 +1
Lines 7303 7369 +66
Branches 1461 1467 +6
==========================================
+ Hits 6677 6743 +66
Misses 626 626
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Great work 🥇
Left a few optional nit comments
plugins/node/opentelemetry-instrumentation-bunyan/src/OpenTelemetryBunyanStream.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-bunyan/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-bunyan/src/OpenTelemetryBunyanStream.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-bunyan/src/OpenTelemetryBunyanStream.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-bunyan/src/OpenTelemetryBunyanStream.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-bunyan/src/OpenTelemetryBunyanStream.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-bunyan/src/instrumentation.ts
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-bunyan/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
@blumamir Thanks for the thoughtful review! I agree with everything you've suggested. I'll need today to re-educate myself on some of this code. :) |
Sure take all the time you need. Feel free to write here if there is something you want to discuss |
@blumamir This is ready for review again, whenever you have a chance. I have questions or thoughts on three of the "conversations" from your initial review above. |
I don't know why My inclination is to wait for the explicit package version handling from #1771 before digging into this too much. |
There was an issue with the CI yesterday due to this, was fixed in contrib by #1779 . Can you please try to rebase? |
@hectorhdzg IIUC, this was considered/discussed earlier on this PR: #1713 (comment) |
Thanks for the reference @trentm, hard to follow up with resolved comments, well then it would be good to have a specific Log Instrumentation then or similar, let use the other PR to discuss it. |
Totally agree. |
Avoid using 'bridge' terminology at suggestion from specs that the Bridge API is an internal detail.
@hectorhdzg Thanks for the review! I've addressed the two terminology changes, and created a separate issue for the log level discussion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for working on this and sticking with us through the review process @trentm 🚀
I'll merge this by tomorrow evening (16:00 GMT+1) 🙂 @seemk (component-owner) any objections? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
My only concern is that it is enabled by default and upgrading the instrumentation in existing pipelines which have the log provider already set up suddenly start sending more (perhaps unwanted) logs. I'm fine with this as long we explicitly mention it in the release notes. Otherwise it's a much needed feature, thanks for all the work! |
Yep I agree, I'll add some info to the squashed commit so that release please adds that info to the changelog 🙂 EDIT: I added a override message hidden in the PR body so that release-please picks it up 🙂 |
Thanks!
@pichlermarc Was there a slight Markdown formatting error in your override block? - * upgrading to this version will start sending logs if a [Logs SDK]([Logs Bridge API](https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/sdk-logs) is registered with the [Logs Bridge API](https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/api-logs)
+ * upgrading to this version will start sending logs if a [Logs SDK](https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/sdk-logs) is registered with the [Logs Bridge API](https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/api-logs) Also, I'm curious, is there a particular reason for putting the override inside an HTML comment to somewhat hide it? |
Yep thanks for catching that. I fixed it. Looks like release-please didn't pick it up though, I must've done something wrong, I'll keep an eye on it and see if it appears. Worst case I'll manually edit the auto-generated github release notes.
Ah I was trying to keep the visible PR body unchanged. The commit override is a bit unsightly. |
This extends the Bunyan instrumentation to automatically add a Bunyan stream to created loggers that will send log records to the Logs Bridge API: https://opentelemetry.io/docs/specs/otel/logs/bridge-api/
Now that the instrumentation supports separate "injection" of fields and "bridging" of log records functionality, this also adds two boolean options to disable those independently:
enableInjection
andenableLogsBridge
.This also updates the instrumentation to work with ES module usage.
Closes: #1559