feat(instrumentation-bunyan): add log sending to Logs Bridge API #1713

trentm · 2023-10-03T17:52:32Z

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: #1559

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 · 2023-10-03T18:26:04Z

Codecov Report

Merging #1713 (ca18034) into main (60d60d0) will increase coverage by 0.07%.
The diff coverage is 98.71%.

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

Files	Coverage Δ
...umentation-bunyan/src/OpenTelemetryBunyanStream.ts	`100.00% <100.00%> (ø)`
...etry-instrumentation-bunyan/src/instrumentation.ts	`98.70% <97.56%> (+0.78%)`	⬆️

blumamir

LGTM
Great work 🥇

Left a few optional nit comments

plugins/node/opentelemetry-instrumentation-bunyan/src/OpenTelemetryBunyanStream.ts

plugins/node/opentelemetry-instrumentation-bunyan/src/instrumentation.ts

plugins/node/opentelemetry-instrumentation-bunyan/src/OpenTelemetryBunyanStream.ts

plugins/node/opentelemetry-instrumentation-bunyan/README.md

plugins/node/opentelemetry-instrumentation-bunyan/src/instrumentation.ts

trentm · 2023-11-08T16:54:42Z

@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. :)

blumamir · 2023-11-08T17:33:56Z

@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

Co-authored-by: Amir Blum <amirgiraffe@gmail.com>

…ity, from blumamir

…ithout the instrumentation

…ned values See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#boolean-value Suggestion from blumamir.

trentm · 2023-11-08T23:59:28Z

@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.

trentm · 2023-11-09T00:38:41Z

I don't know why unit-test (18) is failing now. My total guess is that there is something funky with the versions of deps installed, resulting in getting an @opentelemetry/api mismatch such that a NoopTracerProvider is registered. The "Cache Dependencies" step could be related here.

My inclination is to wait for the explicit package version handling from #1771 before digging into this too much.

blumamir · 2023-11-09T07:41:26Z

I don't know why unit-test (18) is failing now. My total guess is that there is something funky with the versions of deps installed, resulting in getting an @opentelemetry/api mismatch such that a NoopTracerProvider is registered. The "Cache Dependencies" step could be related here.

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 · 2023-11-21T23:23:05Z

Related open-telemetry/opentelemetry-js#4314

trentm · 2023-11-21T23:31:00Z

Related open-telemetry/opentelemetry-js#4314

@hectorhdzg IIUC, this was considered/discussed earlier on this PR: #1713 (comment)

hectorhdzg · 2023-11-21T23:38:10Z

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.

trentm · 2023-11-21T23:39:42Z

hard to follow up with resolved comments

Totally agree.

Avoid using 'bridge' terminology at suggestion from specs that the Bridge API is an internal detail.

trentm · 2023-11-23T19:44:05Z

@hectorhdzg Thanks for the review! I've addressed the two terminology changes, and created a separate issue for the log level discussion.

pichlermarc

Looks great, thanks for working on this and sticking with us through the review process @trentm 🚀

pichlermarc · 2023-11-27T12:38:59Z

I'll merge this by tomorrow evening (16:00 GMT+1) 🙂

@seemk (component-owner) any objections?

hectorhdzg

LGTM

seemk · 2023-11-28T12:19:47Z

I'll merge this by tomorrow evening (16:00 GMT+1) 🙂

@seemk (component-owner) any objections?

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!

pichlermarc · 2023-11-28T15:04:28Z

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.

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 🙂

trentm · 2023-11-28T16:09:54Z

Thanks!

EDIT: I added a override message hidden in the PR body so that release-please picks it up 🙂

@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?

pichlermarc · 2023-11-29T16:27:14Z

Thanks!

EDIT: I added a override message hidden in the PR body so that release-please picks it up 🙂

@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)

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.

Also, I'm curious, is there a particular reason for putting the override inside an HTML comment to somewhat hide it?

Ah I was trying to keep the visible PR body unchanged. The commit override is a bit unsightly.

trentm requested a review from a team October 3, 2023 17:52

github-actions bot assigned seemk Oct 3, 2023

github-actions bot requested a review from seemk October 3, 2023 17:52

Merge branch 'main' into trentm/logs-bridge-bunyan

7d8a21f

trentm mentioned this pull request Oct 3, 2023

Add Bunyan stream for OpenTelemetry #1559

Closed

trentm added 2 commits October 3, 2023 11:09

markdown lint fixes

03c1c16

markdown lint fixes

3fbfc0c

pichlermarc added the pkg:instrumentation-bunyan label Oct 4, 2023

trentm added 3 commits October 15, 2023 12:02

Merge branch 'main' into trentm/logs-bridge-bunyan

57b6628

catch up with recent core-deps update

d5a92d1

some type tweaks suggested by David

5b9aecb

blumamir approved these changes Nov 7, 2023

View reviewed changes

trentm and others added 9 commits November 8, 2023 14:03

more specific type

e3ed22c

Co-authored-by: Amir Blum <amirgiraffe@gmail.com>

use more self-explanatory code for mapping Bunyan level to OTel sever…

bd507e2

…ity, from blumamir

export OpenTelemetryBunyanStream for direct usage in Bunyan loggers w…

45f2b1d

…ithout the instrumentation

.apply over .call suggestion

7d1532f

consistency suggestion

ed9ddaf

suggestion to use the longer (perhaps clearer) logger var name

0f77516

Merge branch 'main' into trentm/logs-bridge-bunyan

aa81256

switch to false-by-default config vars to avoid surprises with undefi…

4c94dba

…ned values See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#boolean-value Suggestion from blumamir.

document using OpenTelemetryBunyanStream without the instrumentation

21e3a55

fix https://eslint.org/docs/latest/rules/prefer-spread lint error

44d20bd

pichlermarc mentioned this pull request Nov 9, 2023

[host-metrics] adapt default meter name to be the npm package name #1782

Closed

hectorhdzg and others added 2 commits November 20, 2023 14:57

Merge branch 'main' into trentm/logs-bridge-bunyan

af2a216

update deps to current releases and sync package-lock.json

f3c4d03

hectorhdzg mentioned this pull request Nov 22, 2023

Allow LoggerProvider to be specified in Instrumentations open-telemetry/opentelemetry-js#4314

Merged

8 tasks

trentm added 2 commits November 22, 2023 12:33

disableInjection -> disableLogCorrelation

c740b76

disableLogsBridge -> disableLogSending

f850a63

Avoid using 'bridge' terminology at suggestion from specs that the Bridge API is an internal detail.

trentm mentioned this pull request Nov 23, 2023

Add configurable logLevel to BunyanInstrumentation (and other logger instrumentations) for custom log sending level #1821

Open

Merge branch 'main' into trentm/logs-bridge-bunyan

45cbb8b

pichlermarc approved these changes Nov 27, 2023

View reviewed changes

Merge branch 'main' into trentm/logs-bridge-bunyan

b597a92

hectorhdzg approved these changes Nov 27, 2023

View reviewed changes

trentm added 3 commits November 27, 2023 10:23

correct the default instrumentation scope name (as discussed earlier)

39b8507

tests: fix test for intrumentationScope.name change in previous commit

a149912

fix lint

ca18034

pichlermarc merged commit 4a1d83c into open-telemetry:main Nov 28, 2023
16 checks passed

dyladan mentioned this pull request Nov 28, 2023

chore: release main #1829

Merged

trentm deleted the trentm/logs-bridge-bunyan branch November 28, 2023 16:10

hectorhdzg mentioned this pull request Nov 30, 2023

feat(instrumentation-winston): add log sending to Logs API #1837

Merged

hectorhdzg mentioned this pull request Dec 11, 2023

Log Bridge API Appender packages or use current Instrumentation packages open-telemetry/opentelemetry-js#4355

Closed

dyladan mentioned this pull request Feb 13, 2024

chore: release main #1935

Merged

trentm mentioned this pull request May 30, 2024

feat(instrumentation-pino): add log sending to Logs Bridge API #2249

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(instrumentation-bunyan): add log sending to Logs Bridge API #1713

feat(instrumentation-bunyan): add log sending to Logs Bridge API #1713

trentm commented Oct 3, 2023 •

edited by pichlermarc

Loading

codecov bot commented Oct 3, 2023 •

edited

Loading

blumamir left a comment

trentm commented Nov 8, 2023

blumamir commented Nov 8, 2023

trentm commented Nov 8, 2023

trentm commented Nov 9, 2023

blumamir commented Nov 9, 2023

hectorhdzg commented Nov 21, 2023

trentm commented Nov 21, 2023

hectorhdzg commented Nov 21, 2023

trentm commented Nov 21, 2023

trentm commented Nov 23, 2023

pichlermarc left a comment

pichlermarc commented Nov 27, 2023 •

edited

Loading

hectorhdzg left a comment

seemk commented Nov 28, 2023

pichlermarc commented Nov 28, 2023 •

edited

Loading

trentm commented Nov 28, 2023 •

edited

Loading

pichlermarc commented Nov 29, 2023

feat(instrumentation-bunyan): add log sending to Logs Bridge API #1713

feat(instrumentation-bunyan): add log sending to Logs Bridge API #1713

Conversation

trentm commented Oct 3, 2023 • edited by pichlermarc Loading

codecov bot commented Oct 3, 2023 • edited Loading

Codecov Report

blumamir left a comment

Choose a reason for hiding this comment

trentm commented Nov 8, 2023

blumamir commented Nov 8, 2023

trentm commented Nov 8, 2023

trentm commented Nov 9, 2023

blumamir commented Nov 9, 2023

hectorhdzg commented Nov 21, 2023

trentm commented Nov 21, 2023

hectorhdzg commented Nov 21, 2023

trentm commented Nov 21, 2023

trentm commented Nov 23, 2023

pichlermarc left a comment

Choose a reason for hiding this comment

pichlermarc commented Nov 27, 2023 • edited Loading

hectorhdzg left a comment

Choose a reason for hiding this comment

seemk commented Nov 28, 2023

pichlermarc commented Nov 28, 2023 • edited Loading

trentm commented Nov 28, 2023 • edited Loading

pichlermarc commented Nov 29, 2023

trentm commented Oct 3, 2023 •

edited by pichlermarc

Loading

codecov bot commented Oct 3, 2023 •

edited

Loading

pichlermarc commented Nov 27, 2023 •

edited

Loading

pichlermarc commented Nov 28, 2023 •

edited

Loading

trentm commented Nov 28, 2023 •

edited

Loading