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 Winston format/transport for OpenTelemetry #1558

Closed
martinkuba opened this issue Jul 5, 2023 · 0 comments · Fixed by #1837
Closed

Add Winston format/transport for OpenTelemetry #1558

martinkuba opened this issue Jul 5, 2023 · 0 comments · Fixed by #1837

Comments

@martinkuba
Copy link
Contributor

Is your feature request related to a problem? Please describe

The JS SDK now has full Logs API/SDK support. The Logs API is intended for creating log appenders for individual logging libraries.

This is a feature request to add support for Winston.

Describe the solution you'd like to see

Users can configure Winston to export logs using to OTLP backends.

@pichlermarc pichlermarc added instrumentation-request up-for-grabs Good for taking. Extra help will be provided by maintainers labels Jul 10, 2023
@hectorhdzg hectorhdzg self-assigned this Nov 20, 2023
@hectorhdzg hectorhdzg removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label Nov 20, 2023
maxdml added a commit to dbos-inc/dbos-transact-ts that referenced this issue Dec 11, 2023
This PR updates the telemetry subsystem to support 1) formatting log
entries to OTLP and 2) export logs/traces to configurable OTLP backends.

For instance, with a service that accepts both traces and logs (like the
OTel collector), the DBOS SDK can be configure to export both signals to
this service:
```yaml
telemetry:
    OTLPExporters:
        - tracesEndpoint: 'http://localhost:4318/v1/traces'
        - logsEndpoint: 'http://localhost:4318/v1/logs'
```
Note this setup works out of the box with Jaeger and the OTel collector
(see screenshots bellow.)
Also note `opentelemetry-js` logs API is experimental, so I fixed the
version we use to `0.41.2`

**Details**
- Use a wrapper around the Winston logger everywhere.
- The wrapper adds a new OTLP transport to the wrapped winston logger if
it detects configured exporters
- This transport simply formats an OTel `LogRecord` and push it to our
collector
- Massively simplify our typing by only using opentelemetry `LogRecord`
and `Span`

**Others**
- Metadata are injected to all Winston transports and the transport
logic decides whether to include them or not

**Thoughts**
- There exists a neater way to integrate the active span context to each
OTLP log records, which we can consider in the future. Talk to me for
details or see PR comments about having a context manager
- The Winston folks are actively working on an OTLP transport/exporter
themselves, see
[here](open-telemetry/opentelemetry-js-contrib#1558)
and
[there](open-telemetry/opentelemetry-js-contrib#1837).
In the future we might want to use it.

**Tests**

☑️ Unit tests: ideally we would be able to intercept outgoing HTTP
requests and expect the payload to conform our expectations. This seems
to be quite some work so pushing that for later.

✅ Tested with DBOS cloud integration tests.

✅ Logs w/o metadata:
![Screenshot 2023-12-11 at 11 57
42](https://github.com/dbos-inc/dbos-ts/assets/3437048/093efe7a-6ff2-4ec0-bf6b-f4d9e30f7ed9)


✅ Logs w/ metadata:
![Screenshot 2023-12-11 at 12 01
02](https://github.com/dbos-inc/dbos-ts/assets/3437048/8a8ed3ff-37a7-4632-aee4-d140bfb0a549)


✅ Logs & traces exported to a local OTel collector
![Screenshot 2023-12-10 at 14 23
25](https://github.com/dbos-inc/dbos-ts/assets/3437048/292acc9e-a7f6-47e5-bb58-f7216d8afcfd)

✅ Traces exported to a local Jaeger:
<img width="1784" alt="jaeger"
src="https://github.com/dbos-inc/dbos-ts/assets/3437048/89b6e8dc-a2da-43c6-9f4f-821ed31c2159">
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.

3 participants