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 Pino transport for OpenTelemetry #2110

Open
mhennoch opened this issue Apr 17, 2024 · 6 comments
Open

Add Pino transport for OpenTelemetry #2110

mhennoch opened this issue Apr 17, 2024 · 6 comments

Comments

@mhennoch
Copy link
Contributor

Is your feature request related to a problem? Please describe

We should add Logs API support to Pino also now that Bunyan/Winston have it.

Describe the solution you'd like to see

Seems like Pino recommends doing this via transports. Ideally transports operate in a separate thread or a separate process which won't have access to globally set log provider. So we would need to duplicate and setup log provider inside the transport itself together with resource detection, processor/exporter configuration etc. I don't see a sane way of passing in custom processor/exporter to the transporter tho. Also need to make sure this works well with Pino instrumentation and auto instrumentation.

Pino already links to one OTEL transport. Maybe @Vunovati is willing to move it to contrib.

Are there any other ways of doing this besides using a transport?

@Vunovati
Copy link

Hello @mhennoch, what problem would moving the transport to this repo solve for the end users? Is there a way to do it while preserving all the contributions of the contributors to the original repo?

@mhennoch
Copy link
Contributor Author

  1. There is more trust in instrumentation that comes directly from upstream. We have customers who refuse to use instrumentations not coming from contrib.
  2. Ideally this transport works with existing Pino instrumentation. Would makes sense for them to be in the same place esp. when some configuration passing might be needed.
  3. Other logging frameworks have them in contrib

Probably not, everyone who has contributed their instrumentation to upstream has lost the history. I actually have a somewhat working version of it. I can contribute that and you can review if it works for you.

@Vunovati
Copy link

I plan to move the repo to pino org https://github.com/pinojs. It was the initial plan when I first created the transport but kept it in a private repo to move things faster.

If now trust is becoming an issue maybe it is time to proceed with the plan. The package was already reviewed by pino maintainers.

This will enable keeping the history while also adressing your concerns. Your clients are trusting pino org so they should trust the transport as well. I will do that next week

P.S. Re trust, you can point your clients that the transport and all its dependencies have provenance https://www.npmjs.com/package/pino-opentelemetry-transport

@Flarna
Copy link
Member

Flarna commented Apr 18, 2024

As long as quite some log related packages (as of now all) are experimental and in the 0.x.y version range release here and dependency update by consumers need to go hand in hand.
e.g. depending on ^0.49.1 does not pick up 0.50.0 so users end up with duplicates.

If it is here this sort of updates/releases are handled once for all here.

Don't know if this is a valid/good reason to add it here.

@Vunovati
Copy link

Hello,

Pino Opentelemetry Transport is now moved to @pinojs org

https://github.com/pinojs/pino-opentelemetry-transport

I created an issue with the description and the motivation for doing so here:
pinojs/pino-opentelemetry-transport#161

Thanks

@seemk
Copy link
Contributor

seemk commented Apr 22, 2024

As long as quite some log related packages (as of now all) are experimental and in the 0.x.y version range release here and dependency update by consumers need to go hand in hand. e.g. depending on ^0.49.1 does not pick up 0.50.0 so users end up with duplicates.

If it is here this sort of updates/releases are handled once for all here.

Don't know if this is a valid/good reason to add it here.

That's a good point to add it to OTel contrib. We're trying to avoid duplicate dependencies as much as possible and having the instrumentation depend on an external release will definitely complicate things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants