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 experimental support for customizing tracer log levels #727

Merged
merged 13 commits into from
Nov 7, 2019

Conversation

ericmustin
Copy link
Contributor

@ericmustin ericmustin commented Oct 27, 2019

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 ericmustin requested a review from a team as a code owner October 27, 2019 17:21
@ericmustin
Copy link
Contributor Author

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 rochdev added core enhancement New feature or request labels Oct 28, 2019
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
packages/dd-trace/src/log.js Outdated Show resolved Hide resolved
packages/dd-trace/src/config.js Outdated Show resolved Hide resolved
packages/dd-trace/src/log.js Outdated Show resolved Hide resolved
packages/dd-trace/src/log.js Outdated Show resolved Hide resolved
@rochdev
Copy link
Member

rochdev commented Oct 28, 2019

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

Copy link
Contributor Author

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

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

docs/API.md Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
packages/dd-trace/src/log.js Outdated Show resolved Hide resolved
packages/dd-trace/src/log.js Outdated Show resolved Hide resolved
packages/dd-trace/src/config.js Outdated Show resolved Hide resolved
packages/dd-trace/src/log.js Show resolved Hide resolved
packages/dd-trace/src/log.js Outdated Show resolved Hide resolved
@ericmustin
Copy link
Contributor Author

@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

Copy link
Member

@rochdev rochdev left a comment

Choose a reason for hiding this comment

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

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

packages/dd-trace/src/log.js Show resolved Hide resolved
packages/dd-trace/src/log.js Outdated Show resolved Hide resolved
packages/dd-trace/src/log.js Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

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

rochdev
rochdev previously approved these changes Nov 6, 2019
@rochdev
Copy link
Member

rochdev commented Nov 6, 2019

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

@rochdev
Copy link
Member

rochdev commented Nov 7, 2019

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.

@rochdev rochdev merged commit 9450a3a into DataDog:master Nov 7, 2019
@rochdev rochdev added this to the 0.16.0 milestone Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants