Add experimental support for customizing tracer log levels by ericmustin · Pull Request #727 · DataDog/dd-trace-js

ericmustin · 2019-10-27T17:21:17Z

What does this PR do?

Following up on Feedback from PR-718 This PR addresses the use case introduced in Issue 599 for when a user wants to forward tracer errors to their logger but ignore debug messages.

This PR adds experimental support for an ~~experimental.ddTraceLogLevels~~ experimental.logLevel configuration option as well as DD_TRACE_LOG_LEVEL environment variable, which allow a user to specify the ~~specific~~ minimum log levels that they'd like to permit from the tracer's logger. It accepts the format of ~~an array of strings such as ['error','debug','info'] as well as a comma separated value string such as 'error,info'~~ a string such as 'error' or 'debug'.

Motivation

Implementing feedback discussed from PR-718

Additional Notes

I tried to incorporate the feedback from PR-718 in this pull request. A few notes.

Some of the implementation is a bit clunky given that I am trying to target a future logger that hasn't been written or spec'd out, so I tried to leave things flexible enough that they can adjusted if there is a larger refactor of the default logger in the future. At a high level my approach was to give the user a few ways to set the tracer log levels they'd want logged, and then ideally not have to worry about changing that format if there's a larger refactor in the future / new log levels are added.
There's some helper methods for determining if the specific log level is enabled or not (_isLogLevelEnabled ) that I'd be open to feedback on if there's a better way to approach this. I tried to write a method that would automagically determine the log level's function name that was invoking it and check if that name had been whitelisted, however the approach to doing so seemed to rely on either outdated js concepts (using something like arguments.callee ) or methods that might not play nicely when minified (specifically myFunction.name had some scary language around it in this S/O Note ). So the current approach is a bit clunky.
Unclear how i ought to document this but happy to add docs/usage example to api/docs.md in this PR (i made the option experimental for now and it doesn't look like we really update that api/docs.md file until an option is "non-experimental").
It's entirely possible i botched the typescript part of this, apologies if so, still need to level up with my TS.
Any and all feedback is welcome, and generally if it's better to shelve this until there's more clarity on what logging might look like after additional log lvls have been added, thats ok too. Just trying to unblock the folks who'd been asking for it.

ericmustin · 2019-10-28T10:13:15Z

Looks like node-tedious tests are failing, I assume related to what's described in PR-726 , happy to update this branch once that work has been merged into master

rochdev · 2019-10-28T19:04:00Z

For the API docs, it should be enough to just add an entry in API.md for the new configuration option.

ericmustin

ergh not sure why this started a review for me but, updated PR from feedback, thanks again for taking a look!

ericmustin · 2019-11-01T11:36:41Z

@rochdev updated with feedback again, i think i've covered everything and appreciate the patience with all the back and forth here, feels like we're close. also my browser tests are failing for reasons i'm not entirely clear about

rochdev

A few small things but then it's good to go.

ericmustin

updated with feedback, still unclear why browser tests are failing though

rochdev · 2019-11-06T17:37:24Z

@ericmustin Can you try to rebase? Hopefully this will fix the browser tests since I don't see why they would fail.

…mental options, add tests

…docs

…mental options, add tests

…docs

rochdev · 2019-11-07T16:24:50Z

It looks like the build issue is because of BrowserStack credentials in CircleCI from a fork. I'll go ahead and merge it and if my theory is correct I'll contact CircleCI.

#16174) ## Description  Today, the ddtrace logger follows the root logger. If the root logger is in DEBUG mode, then ddtrace will also emit logs and the only way to override that is to set something like this: https://docs.datadoghq.com/tracing/troubleshooting/tracer_debug_logs/?tab=python#review-debug-logs ``` import logging # root logger configuration root_logger = logging.getLogger() root_logger.setLevel(logging.DEBUG) # override the ddtrace configuration to WARNING log level logging.getLogger("ddtrace").setLevel(logging.WARNING) ``` This change introduces a DD_TRACE_LOG_LEVEL setting that follows the logging levels in the logging module: https://docs.python.org/3/library/logging.html#levels . `DD_TRACE_DEBUG` will take precedence if set and set the tracer to debug. However, if DD_TRACE_LOG_LEVEL is used as a standalone, it can control the ddtrace logger setting. Example `DD_TRACE_LOG_LEVEL=CRITICAL` to only get critical logs instead of everything. In startup logs, I have the setting report as `log_level_override` to avoid confusion with the debug mode option someone may be trying to set: ``` - DATADOG TRACER CONFIGURATION - {... 'debug': True, ..... 'log_level_override': None} ``` Addresses APMS-18427 ## Testing  Added tests that assert on the order of DD_TRACE_DEBUG and DD_TRACE_LOG_LEVEL . ## Risks  There really isn't a true "NO LOGS" setting if we follow these [logging levels](https://docs.python.org/3/library/logging.html#levels), so if we accept this PR, we need to avoid setting any critical logs unless it's an exceptional use case. ## Additional Notes  There have been various changes to how debug mode can be set over the years. Some questions I think will be asked of the reviewers and here's my answer: 1. Do other tracers have this setting? Yes. Java has DD_LOG_LEVEL: DataDog/dd-trace-java#7159 Node.js has DD_TRACE_LOG_LEVEL: DataDog/dd-trace-js#727 2. If DD_TRACE_DEBUG=debug, how is it different than setting DD_TRACE_LOG_LEVEL=debug? Won't they have the same problems? All DD_TRACE_DEBUG does today is enables debug mode if the user sets it to TRUE: https://github.com/DataDog/dd-trace-py/blob/c173a8a084b0f7f2a8cd081dbf9611208faa48e0/ddtrace/_logger.py#L53-L54. Setting it to "FALSE" has no effect if the root logger has already been configured. **DD_TRACE_LOG_LEVEL** is different in the sense that it allows us to override the log level setting beyond accepting DEBUG mode or accepting whatever the root logger's level was. --------- Co-authored-by: Munir Abdinur <[email protected]> Co-authored-by: Quinna Halim <[email protected]>

ericmustin requested a review from a team as a code owner October 27, 2019 17:21

rochdev added core enhancement labels Oct 28, 2019

rochdev requested changes Oct 28, 2019

View reviewed changes

Comment thread index.d.ts Outdated

Comment thread index.d.ts Outdated

Comment thread packages/dd-trace/src/log.js Outdated

Comment thread packages/dd-trace/src/config.js Outdated

Comment thread packages/dd-trace/src/log.js Outdated

Comment thread packages/dd-trace/src/log.js Outdated

ericmustin commented Oct 29, 2019

View reviewed changes

rochdev requested changes Oct 30, 2019

View reviewed changes

rochdev requested changes Nov 4, 2019

View reviewed changes

Comment thread packages/dd-trace/src/log.js Outdated

Comment thread packages/dd-trace/src/log.js Outdated

Comment thread packages/dd-trace/src/log.js Outdated

Comment thread index.d.ts Outdated

ericmustin commented Nov 5, 2019

View reviewed changes

rochdev previously approved these changes Nov 6, 2019

View reviewed changes

ericmustin dismissed rochdev’s stale review via b21f1a1 November 7, 2019 09:25

ericmustin added 12 commits November 7, 2019 11:22

add support for custom log level specification via env var and experi…

7d94ec2

…mental options, add tests

fix specs

5147f19

stub out console.warn for tests

9160238

refactor to use min logLevel and LOG_LEVEL env var, update tests and …

c68473b

…docs

update to use dd_trace_log_level env var and improve perf for validation

713807e

update initialization to point to non experimental config option

1fe3df9

feedback, add ts test and use bunyan log levels

bea9d20

remove unneeded check

eefca4a

add support for custom log level specification via env var and experi…

94a92f6

…mental options, add tests

stub out console.warn for tests

e1a8f44

refactor to use min logLevel and LOG_LEVEL env var, update tests and …

9721f93

…docs

update to use dd_trace_log_level env var and improve perf for validation

9c8e444

ericmustin force-pushed the 559_log_env_var branch from 8db71ba to 9c8e444 Compare November 7, 2019 10:47

remove whitespace

a88812c

rochdev approved these changes Nov 7, 2019

View reviewed changes

rochdev merged commit 9450a3a into DataDog:master Nov 7, 2019

rochdev mentioned this pull request Jul 28, 2021

Decouple enablement of error and debug logs #559

Closed

rochdev added this to the 0.16.0 milestone Jul 28, 2021

wantsui mentioned this pull request Jan 22, 2026

feat(logging): add DD_TRACE_LOG_LEVEL to control the ddtrace log level DataDog/dd-trace-py#16174

Merged

Uh oh!

Conversation

ericmustin commented Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Uh oh!

ericmustin commented Oct 28, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rochdev commented Oct 28, 2019

Uh oh!

ericmustin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericmustin commented Nov 1, 2019

Uh oh!

rochdev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericmustin left a comment

Choose a reason for hiding this comment

Uh oh!

rochdev commented Nov 6, 2019

Uh oh!

rochdev commented Nov 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ericmustin commented Oct 27, 2019 •

edited

Loading