chore(logging)!: set default value of DD_CALL_BASIC_CONFIG to False #3168
Conversation
|
@mabdinur this pull request is now in conflict 😩 |
mabdinur
changed the title
|
@mabdinur this pull request is now in conflict 😩 |
mabdinur
force-pushed
the
munir/disable-logging
branch
from
b9654fd
to
9830787
Compare
mabdinur
changed the title
mabdinur
changed the title
mabdinur
changed the title
mabdinur
changed the title
There was a problem hiding this comment.
The change looks good to me. The one concern I have is whether or not we want this to be included in 1.0.
The philosophy of 1.0 was to privatize/hide as much implementation detail as possible. While we definitely want to make this change I'm not sure if we want to add it to 1.0 as it could add additional overhead to upgrading.
I think once we have the almost complete list of release notes for 1.0 we can evaluate whether this change should be included or not.
@brettlangdon @mabdinur thoughts?
An alternative idea is to just change the default of |
After discussing this offline we'll move forward with removing |
There was a problem hiding this comment.
I can't remember if we discussed this, but I wonder if we should keep the code as-is, but just change DD_CALL_BASIC_CONFIG default to False.
Keeping the env variable means if someone was relying on it then there is at least a feature flag they can toggle to get the behavior back.
However, keeping it also means we should get rid of it in the future.
What do others think?
mabdinur
force-pushed
the
munir/disable-logging
branch
from
ea1638a
to
5f7c9bf
Compare
|
@mabdinur this pull request is now in conflict 😩 |
mabdinur
force-pushed
the
munir/disable-logging
branch
from
19a905f
to
7025b0a
Compare
mabdinur
changed the title
brettlangdon
changed the title
releasenotes/notes/ddtrace-1.0-disable-basic-config-call-by-default-b677844e4a13a794.yaml
Outdated
Show resolved
Hide resolved
mabdinur
force-pushed
the
munir/disable-logging
branch
2 times, most recently
from
6e7fa7e
to
ff69c23
Compare
mabdinur
force-pushed
the
munir/disable-logging
branch
3 times, most recently
from
5990c5e
to
6901d21
Compare
…nd log deprecation if it's set to true
mabdinur
force-pushed
the
munir/disable-logging
branch
from
6901d21
to
a6bb9a3
Compare
…3168) ddtrace should not call `logging.basicConfig` since it is not a logging library. This PR updates the default value of ``DD_CALL_BASIC_CONFIG`` to False and logs a deprecation warning if DD_CALL_BASIC_CONFIG configuration is set to True. ## Checklist - [ ] Added to the correct milestone. - [ ] Tests provided or description of manual testing performed is included in the code or PR. - [ ] Library documentation is updated. - [ ] [Corp site](https://github.com/DataDog/documentation/) documentation is updated (link to the PR).
…3168) ddtrace should not call `logging.basicConfig` since it is not a logging library. This PR updates the default value of ``DD_CALL_BASIC_CONFIG`` to False and logs a deprecation warning if DD_CALL_BASIC_CONFIG configuration is set to True. ## Checklist - [ ] Added to the correct milestone. - [ ] Tests provided or description of manual testing performed is included in the code or PR. - [ ] Library documentation is updated. - [ ] [Corp site](https://github.com/DataDog/documentation/) documentation is updated (link to the PR).
…3168) ddtrace should not call `logging.basicConfig` since it is not a logging library. This PR updates the default value of ``DD_CALL_BASIC_CONFIG`` to False and logs a deprecation warning if DD_CALL_BASIC_CONFIG configuration is set to True. ## Checklist - [ ] Added to the correct milestone. - [ ] Tests provided or description of manual testing performed is included in the code or PR. - [ ] Library documentation is updated. - [ ] [Corp site](https://github.com/DataDog/documentation/) documentation is updated (link to the PR).
## Summary This PR removes the `DD_CALL_BASIC_CONFIG` environment variable and `DD_LOG_FORMAT` constant from the public API, and features a subtle change in the library's logging functionality to avoid configuring the user application's root-level logger. Moving forward, we will configure the `ddtrace` level logger instead. Additionally, this PR adds a new default behavior to ensure that ddtrace will always log to both stdout (stream) and/or file if configured to log to file. Previously ddtrace would only log to stream OR file, but this isn't intuitive for users. ## Background `DD_CALL_BASIC_CONFIG` was deprecated as of #3168 (February 2022). This environment variable is currently used to enable the ddtrace library to call `logging.basicConfig()` to configure and format the root logger to print logs to stdout (when being run in `ddtrace-run` or through tracer debugging mode). However, the ddtrace library is not a logging library and calling `logging.basicConfig()` will configure the root-level logger and invalidate any future calls to `logging.basicConfig()`, meaning this will potentially interfere with user logging configuration. User applications should be responsible for calling `logging.basicConfig()` instead of ddtrace. ## Proposed Solution Instead of calling loggging.basicConfig(), we instead now configure the ddtrace level logger at module startup (no longer at sitecustomize.py or tracer startup) to add a stdout Streamhandler (or file logger if specified by `DD_TRACE_LOG_FILE`) to avoid interfering with root-level logger configurations. Note that we still configure logging injection at sitecustomize.py and tracer startup since that requires logging to be patched. Note: this PR leaves one usage of `logging.basicConfig()` in `ddtrace/commands/ddtrace_run.py`. This will not impact user application logging because our `ddtrace-run` script replaces the currently running process with the bootstrap script and user application through `os.execl(...)`, meaning this will reset a new logging configuration to default and be unaffected once the bootstrap sitecustomize and user application starts running. This approach will leave ddtrace-run debugging logs to be printed to stdout but still avoids interfering with user application logging configurations. ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. - [x] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
ddtrace should not call
logging.basicConfigsince it is not a logging library.This PR updates the default value of
DD_CALL_BASIC_CONFIGto False and logs a deprecation warning if DD_CALL_BASIC_CONFIG configuration is set to True.Checklist