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

Add Bunyan stream for OpenTelemetry #1559

Closed
martinkuba opened this issue Jul 5, 2023 · 8 comments · Fixed by #1713
Closed

Add Bunyan stream for OpenTelemetry #1559

martinkuba opened this issue Jul 5, 2023 · 8 comments · Fixed by #1713

Comments

@martinkuba
Copy link
Contributor

Is your feature request related to a problem? Please describe

This is a feature request to build a package to export Bunyan logs to OpenTelemetry backends. This would likely be done as a custom Bunyan stream utilizing the OpenTelemetry Logs API.

Describe the solution you'd like to see

Users can configure Bunyan to export logs to OpenTelemetry backends.

Related to #1558

@trentm
Copy link
Contributor

trentm commented Jul 10, 2023

Here is a proof-of-concept for this, as a start. I have a number of open questions and TODOs, but I'd appreciate feedback. If/when this seems a reasonable direction, then I can work on a PR to this repo for this. @hectorhdzg You also mentioned you were interested in this.

https://github.com/trentm/otellogsplay

Usage looks like this:

// ...
var log = bunyan.createLogger({name: 'myapp'});
log.addStream({
  type: 'raw',
  stream: new OTelBunyanAppender()
})

And that "appender" implementation is here: https://github.com/trentm/otellogsplay/blob/main/lib/OTelBunyanAppender.js

Here is an example run of that script:
% node bunyan-otel.js
{"name":"myapp","hostname":"purple.local","pid":46450,"level":30,"msg":"hi","time":"2023-07-10T23:16:17.930Z","v":0}
{
  timestamp: 1689030977930000,
  traceId: undefined,
  spanId: undefined,
  traceFlags: undefined,
  severityText: 'info',
  severityNumber: 9,
  body: 'hi',
  attributes: { name: 'myapp', hostname: 'purple.local', pid: 46450 }
}
{"name":"myapp","hostname":"purple.local","pid":46450,"level":30,"msg":"in a span","time":"2023-07-10T23:16:17.941Z","v":0}
{
  timestamp: 1689030977941000,
  traceId: '2891fcee99788edfabfa3c344edb70f8',
  spanId: '81b5a5a8de16a34d',
  traceFlags: 1,
  severityText: 'info',
  severityNumber: 9,
  body: 'in a span',
  attributes: { name: 'myapp', hostname: 'purple.local', pid: 46450 }
}
{
  traceId: '2891fcee99788edfabfa3c344edb70f8',
  parentId: undefined,
  traceState: undefined,
  name: 'manual-span',
  id: '81b5a5a8de16a34d',
  kind: 0,
  timestamp: 1689030977940000,
  duration: 1946,
  attributes: {},
  status: { code: 0 },
  events: [],
  links: []
}

Does this look like a reasonable start?
If so, I have a number of open questions and TODOs in the README and in https://github.com/trentm/otellogsplay/blob/main/lib/OTelBunyanAppender.js I'm happy to discuss them here, in a PR, or on Slack.

@hectorhdzg
Copy link
Member

@trentm I think this is a good start, it would be easier to provide feedback directly in a PR, I think it would be good to align as much as possible with what Instrumentations are doing, maybe create similar abstract class, use global LoggerProvider by default, expose setLoggerProviderMethod, enabled/disable funcitonality, getLogger receiving LoggerOptions, etc.

@trentm
Copy link
Contributor

trentm commented Jul 13, 2023

Thanks for taking a look.

it would be easier to provide feedback directly in a PR

Understood. One thing I'm confused/struggling with is where this should live. Would this be a package, say at opentelemetry-js-contrib/packages/[opentelemetry-]bunyan-log-appender? A comment on this weeks SIG call and your mention of Instrumentations makes me wonder if some think this should be an instrumentation -- i.e. if it would fit in with the existing opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-bunyan.

My initial thought was the former -- that usage of this would be as follows, so a user could explicitly setup their logger to send to whatever OTel SDK has been setup:

const bunyan = require('bunyan');
const { BunyanAppender } = require('@opentelemetry/bunyan-log-appender'); // or whatever package name

const log = bunyan.createLogger({name: 'myapp'});
log.addStream({
  type: 'raw',
  stream: new BunyanAppender(/* ... */)
})

And then possibly the opentelemetry-instrumentation-bunyan instrumentation would change to support automatically calling .addStream(...) on created Bunyan loggers.

Thoughts?

@hectorhdzg
Copy link
Member

Yeah I'm more inclined into having a single bunyan package that provides trace injection functionality and auto generation of Log telemetry through the appender in same place, I can see having two different packages causing a lot of confusion to users, @pichlermarc what do you think?

@martinkuba
Copy link
Contributor Author

I think it makes sense to include trace context injection in this module, so users can install just one package and get all the benefits of OTel. With that said, I think the existing bunyan instrumentation will need to continue to exist for use cases where users don't want to send the logs to OTLP backends.

As a side note, I think the name of the package/class should be stream, not appender, to be consistent with Bunyan's terminology and other existing streams.

@dyladan
Copy link
Member

dyladan commented Jul 19, 2023

There are 2 concerns with logs:

  1. Enrichment with otel context
  2. Capturing for OTLP export

I think it would be helpful if the logs SIG provided guidance around which of these concerns (or both) instrumentations should be handling. Should it be configurable?

@trentm
Copy link
Contributor

trentm commented Jul 26, 2023

(FYI: I haven't made progress on this because I was on vacation last week and am away this week.)

trentm added a commit to trentm/opentelemetry-js-contrib that referenced this issue Oct 3, 2023
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
@trentm
Copy link
Contributor

trentm commented Oct 3, 2023

I have a PR now for this at #1713
It adds to the existing @opentelemetry/instrumentation-bunyan.

@hectorhdzg hectorhdzg removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label Nov 20, 2023
pichlermarc pushed a commit that referenced this issue Nov 28, 2023
* feat(instrumentation-bunyan): add log sending to Logs Bridge API

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

* markdown lint fixes

* markdown lint fixes

* catch up with recent core-deps update

* some type tweaks suggested by David

* more specific type

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

* use more self-explanatory code for mapping Bunyan level to OTel severity, from blumamir

* export OpenTelemetryBunyanStream for direct usage in Bunyan loggers without the instrumentation

* .apply over .call suggestion

* consistency suggestion

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

* switch to false-by-default config vars to avoid surprises with undefined 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

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

* drop options to OpenTelemetryBunyanStream constructor because YAGNI

* temporarily drop CI caching to test theory on unit-test (18) CI failure

* more CI debugging: restore cache, add some 'npm ls -a' to look into NoopContextManager being used

* elide Bunyan 'pid' and 'hostname' fields in OTel log record attributes

Because they are redundant with 'process.pid' and 'host.name'
resource attributes. Add some docs on how to use resource detectors
to the example, because the HostDetector is not on by default in
the NodeSDK.

* update test for having elided 'pid' and 'hostname' fields

* CI debugging: ignore the 'npm ls -a' exit status, they shouldn't break the build

* fix lint and compile errors

* CI debugging: turn on diag DEBUG to test a theory

* turn off diag in this example

* undo CI debugging changes

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

* disableInjection -> disableLogCorrelation

* disableLogsBridge -> disableLogSending

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

* correct the default instrumentation scope name (as discussed earlier)

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

* fix lint

---------

Co-authored-by: Amir Blum <amirgiraffe@gmail.com>
Co-authored-by: Hector Hernandez <39923391+hectorhdzg@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants