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

contrib/log/slog: avoid logging 0 value trace/span IDs #2994

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rliebz
Copy link

@rliebz rliebz commented Nov 26, 2024

If the tracer is started but explicitly deactivated, there will be a span on the context, but the span ID and trace ID will be set to 0. Adding this information to log outputs seems like unintentional noise (although it's certainly possible there's a use case I'm not aware of), so I figured disabling it would be preferred.

The change to explicitly add tracer.WithTraceEnabled(true) was so that the test would be resilient to folks who might have DD_TRACE_ENABLED set explicitly, which is true on my machine.

What does this PR do?

Removes dd.trace_id=0 and dd.span_id=0 values from logs when the tracer is disabled.

Motivation

I was actually using a closed-source implementation of a log/slog handler from before this package was available. I swapped this version into our test suite to compare behavior, and figured this was probably something that could be upstreamed.

It's unclear to me that there would be value in including these zero values in the logs.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

If the tracer is started but explicitly deactivated, there will be a
span on the context, but the span ID and trace ID will be set to 0.
Adding this information to log outputs seems like unintentional noise
(although it's certainly possible there's a use case I'm not aware of),
so I figured disabling it would be preferred.

The change to explicitly add `tracer.WithTraceEnabled(true)` was so that
the test would be resilient to folks who might have `DD_TRACE_ENABLED`
set explicitly, which is true on my machine.
@rliebz rliebz requested review from a team as code owners November 26, 2024 20:24
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.

1 participant