pinojs / pino-opentelemetry-transport

OpenTelemetry transport for Pino
MIT License
24 stars 4 forks source link

Use log-sdk/api for emitting logs instead of otlp-logger #181

Closed pragmaticivan closed 2 months ago

pragmaticivan commented 2 months ago

Any chance you can use the standard channels for emitting/sending logs instead of the current approach?

Vunovati commented 2 months ago

Hello,

otlp-logger is using @opentelemetry/sdk-logs and @opentelemetry/api-logs

What did you have in mind as the standard channels? Can you show me some examples of code using them and the documentation?

pragmaticivan commented 2 months ago

Because I was on a time crunch with a side research, I ended up just writing the whole prototype in a new repo:

https://github.com/pragmaticivan/otel-pino-transport

Happy to backport anything you find useful there.

Vunovati commented 2 months ago

Hey, thanks for the link. I see there aren't any tests in the repo. Pino is running the transport in a dedicated worker thread so it is hard to test in a single process. Here is an explanation related to a similar issue https://github.com/pinojs/pino-opentelemetry-transport/issues/95#issuecomment-1810799377

I wanted to make the instantiation of everything otel-related serializable and decoupled from pino itself. This way, I can test all the options more easily. Also, it enables reuse of this code for CLIs or transports for other loggers

Both this repo and otlp-logger are integration tested with an opentelemetry collector and have 100% test coverage.

When you add tests to your repo, you will encounter similar problems and might converge on a design similar to this. If you come up with a better approach, please notify me.

pragmaticivan commented 2 months ago

Hi, I mainly created that repo to play around with aws lambdas.

So, I'm not even sure If I will maintain it long term.

The reason I'm trying to use api-logs is because I do not want this lib to be in charge of sending the logs, but instead just integrate well enough with the otel ecossystem. Which might not be what you want after all.

I have this WIP PR adding support for automatic transport in the nodejs otel lambda: https://github.com/open-telemetry/opentelemetry-lambda/pull/1464 so I do not want the lib to also abe sending the logs since I want it to be centralized at the collector level

pragmaticivan commented 2 months ago

This is pretty much the behavior I wanted: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/packages/winston-transport

Vunovati commented 2 months ago

Thanks for the explanation. I was unaware of this use case.

You cannot have the same SDK instance managing other OTEL signals and use a Pino transport since pino transport runs in different memory space.

So, the problem is not that this library is not integrated with the Otel ecosystem; the problem is that Pino transports run in a different memory space than the SDK instance you want to integrate with. The two are not integrated intentionally, by design (of pino)

So yes, if you want traces, metrics and logging, you will have two SDK instances running. One managing the traces and metrics, and one in a different worker thread managing the logs.

Winston is not doing a worker thread thing so it works like you want it to.

pragmaticivan commented 2 months ago

That's what I ended up figuring out after some playground time, and one of the reasons I might not be able to pursue this further even on my own repo due to time constraints

pragmaticivan commented 2 months ago

Btw, I do recommend you make some changes on the interface of this project though, so you can instantiate it without using the default mathod (string) and use an imported function instead.

Vunovati commented 2 months ago

Btw, I do recommend you make some changes on the interface of this project though, so you can instantiate it without using the default mathod (string) and use an imported function instead.

Thanks for the suggestion. Can you give an example of how that would look like? In general all the params need to be serializable.