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

Reduce OpenTelemetry warnings #1125

Merged
merged 3 commits into from
Jul 11, 2024
Merged

Reduce OpenTelemetry warnings #1125

merged 3 commits into from
Jul 11, 2024

Conversation

rowanmanning
Copy link
Member

This reduces warnings from the OpenTelemetry package:

  • Disable the pino instrumentation:

    The pino instrumentation is causing us issues because we import our logger before the instrumentation can happen. It's logging an obnoxious warning. I think this is fine to disable because:

    1. We use pino to log to stdout so we're not missing a lot of useful info about the way it runs

    2. The instrumentation only adds traces, not metrics. We run pino async so it doesn't block further execution and shouldn't impact the timings of the routes etc that use it

    3. By default, this instrumentation tries to use the OpenTelemetry logs SDK. I'd rather not load some stuff that we don't want to use.

  • Wrap the Reliability Kit logger:

    We have been losing information from the OpenTelemetry internal logs because they log multiple messages in a row which pino/reliability kit does not support. We don't support this because we want to maintain consistency with n-logger.

    To fix this, we need to wrap the Reliability Kit logger and extract any extra logged information into a new property. I've gone with details. Testing this locally, it's not common to get any info but it's important when we do get it - it helped me debug the issue with loading Pino ahead of instrumentation.

  • Manually provide a meter to HostMetrics

    This suppresses an annoying warning. The host metrics class already gets the global meter provider in the same way but logs a warning if it has to. This is a lot of noise in our logs.

What it doesn't do:

@rowanmanning rowanmanning requested a review from kavanagh July 10, 2024 13:45
@rowanmanning rowanmanning requested a review from a team as a code owner July 10, 2024 13:45
@rowanmanning rowanmanning enabled auto-merge (rebase) July 10, 2024 13:56
The pino instrumentation is causing us issues because we import our
logger before the instrumentation can happen. It's logging an obnoxious
warning. I think this is fine to disable because:

1. We use pino to log to stdout so we're not missing a lot of useful
   info about the way it runs

2. The instrumentation only adds traces, not metrics. We run pino async
   so it doesn't block further execution and shouldn't impact the
   timings of the routes etc that use it

3. By default, this instrumentation tries to use the OpenTelemetry logs
   SDK. I'd rather not load some stuff that we don't want to use.
We have been losing information from the OpenTelemetry internal logs
because they log multiple messages in a row which pino/reliability kit
does not support. We don't support this because we want to maintain
consistency with n-logger.

To fix this, we need to wrap the Reliability Kit logger and extract any
extra logged information into a new property. I've gone with `details`.
Testing this locally, it's not common to get any info but it's important
when we do get it - it helped me debug the issue with loading Pino ahead
of instrumentation.
This suppresses an annoying warning. The host metrics class already gets
the global meter provider in the same way but logs a warning if it has
to. This is a lot of noise in our logs.
@rowanmanning rowanmanning force-pushed the reduce-otel-warnings branch from b970fd9 to 9d1b587 Compare July 11, 2024 15:34
Copy link
Contributor

@CyntiBinti CyntiBinti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying the otels internal logging process 🙇🏽

@rowanmanning rowanmanning merged commit 63c8216 into main Jul 11, 2024
14 checks passed
@rowanmanning rowanmanning deleted the reduce-otel-warnings branch July 11, 2024 15:40
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

Successfully merging this pull request may close these issues.

2 participants