DDLogger doesn't respect logging hierarchy, and is not multi-process safe #4856

mattalbr · 2023-01-04T00:20:38Z

Summary of problem

ddtrace.internal.logger directly instantiates its own logging.Logger instances, which is explicitly disallowed (in all caps and italics to boot) in the official python documentation. Because of this, it has at least a few uncaught bugs:

It modifies an internal data structure of the logging manager (manager.loggerDict) without acquiring and releasing the lock, making it not multi-process safe
It doesn't properly update the logging tree when inserting loggers above a logger that already exists. Put simply, it calls logging.Manager._fixupParents() (which is already nefariously accessing python library internals), but never logging.Manager._fixupChildren()

Both bugs are easily fixable by doubling down on re-implementing cpython internals and making sure to call _acquireLock, _releaseLock, and _fixupChildren when appropriate. As a user of this library, that's enough to keep me happy.

As an unsolicited opinion though, I think accessing cpython internals is a Bad Idea (TM) and the logic here could easily be implemented via a subclass of logging.Filter. logging.Filter could implement the fixed-window rate limiting logic and the api even allows for modification of the LogRecord, so it could also append the %s additional messages skipped to the msg. That said, I have no skin in the game, so if you go the _fixupChildren route, that's none of my business.

Which version of dd-trace-py are you using?

1.6.4

Which version of pip are you using?

22.2.2

Which libraries and their versions are you using?

N/A

How can we reproduce your problem?

I wrote a test to repro the hierarchy because it's easier and was what initially brought me here. The multiprocessing bug is more challenging to write a test for, but is pretty clear from reading the code and comparing to cpython.

def test_dd_logger_inheritance(self):
    root = logging.getLogger()
    child_logger = get_logger("some.long.internal.logger")
    self.assertEqual(child_logger.parent, root)

    parent_logger = get_logger("some.long")
    self.assertEqual(child_logger.parent, parent_logger)  # This fails.

    parent_logger.setLevel(logging.INFO)
    self.assertEqual(child_logger.getEffectiveLevel(), logging.INFO)  # This also fails if only this test is run (root logger defaults to WARNING), but should probably be a little more robust for a test that you actually check in.

What is the result that you get?

child_logger.parent is still set to the root logger.

In practice, this came up because ddtrace was spamming debug logs (we have the root logger set to debug in our dev environment) despite me calling logging.getLogger("ddtrace").setLevel(logging.WARNING)

What is the result that you expected?

Setting the log level on a parent of any child logger should change the effective log level of the child.

The text was updated successfully, but these errors were encountered:

DDLogger is essentially a reimplementation of cPython's logger, but only called _fixupParents to cleanup PlaceHolders in the logger tree, not _fixupChildren. This led to bugs inheriting log level as demonstrated in the new unit tests. I filed #4856 5 months ago, but never received any comments on it. Personally, I disagree with the decision to re-implement cPython's logger by calling cPython internals. In the issue I filed, I describe a reasonable alternative using exclusively the public API. But, that's a bigger project, and I just need the ability to silence ddtrace logs in my application. So for now, I left the implementation as-is and brought both the code structure and functionality in line with the cPython implementation it's imitating. ## 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/contributing.html#Release-Note-Guidelines) are followed. - I don't have all of ddtrace's tooling installed. If someone could help me generate the release notes that would be awesome. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). ## 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. --------- Co-authored-by: Zachary Groves <[email protected]> Co-authored-by: Tahir H. Butt <[email protected]> Co-authored-by: Emmett Butler <[email protected]>

DDLogger is essentially a reimplementation of cPython's logger, but only called _fixupParents to cleanup PlaceHolders in the logger tree, not _fixupChildren. This led to bugs inheriting log level as demonstrated in the new unit tests. I filed #4856 5 months ago, but never received any comments on it. Personally, I disagree with the decision to re-implement cPython's logger by calling cPython internals. In the issue I filed, I describe a reasonable alternative using exclusively the public API. But, that's a bigger project, and I just need the ability to silence ddtrace logs in my application. So for now, I left the implementation as-is and brought both the code structure and functionality in line with the cPython implementation it's imitating. ## 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/contributing.html#Release-Note-Guidelines) are followed. - I don't have all of ddtrace's tooling installed. If someone could help me generate the release notes that would be awesome. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). ## 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. --------- Co-authored-by: Zachary Groves <[email protected]> Co-authored-by: Tahir H. Butt <[email protected]> Co-authored-by: Emmett Butler <[email protected]> (cherry picked from commit 11a8d09)

… 1.16] (#6351) Backport 11a8d09 from #6080 to 1.16. DDLogger is essentially a reimplementation of cPython's logger, but only called _fixupParents to cleanup PlaceHolders in the logger tree, not _fixupChildren. This led to bugs inheriting log level as demonstrated in the new unit tests. I filed #4856 5 months ago, but never received any comments on it. Personally, I disagree with the decision to re-implement cPython's logger by calling cPython internals. In the issue I filed, I describe a reasonable alternative using exclusively the public API. But, that's a bigger project, and I just need the ability to silence ddtrace logs in my application. So for now, I left the implementation as-is and brought both the code structure and functionality in line with the cPython implementation it's imitating. ## 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/contributing.html#Release-Note-Guidelines) are followed. - I don't have all of ddtrace's tooling installed. If someone could help me generate the release notes that would be awesome. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). ## 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. Co-authored-by: mattalbr <[email protected]>

… 1.15] (#6350) Backport 11a8d09 from #6080 to 1.15. DDLogger is essentially a reimplementation of cPython's logger, but only called _fixupParents to cleanup PlaceHolders in the logger tree, not _fixupChildren. This led to bugs inheriting log level as demonstrated in the new unit tests. I filed #4856 5 months ago, but never received any comments on it. Personally, I disagree with the decision to re-implement cPython's logger by calling cPython internals. In the issue I filed, I describe a reasonable alternative using exclusively the public API. But, that's a bigger project, and I just need the ability to silence ddtrace logs in my application. So for now, I left the implementation as-is and brought both the code structure and functionality in line with the cPython implementation it's imitating. ## 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/contributing.html#Release-Note-Guidelines) are followed. - I don't have all of ddtrace's tooling installed. If someone could help me generate the release notes that would be awesome. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). ## 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. Co-authored-by: mattalbr <[email protected]>

DDLogger is essentially a reimplementation of cPython's logger, but only called _fixupParents to cleanup PlaceHolders in the logger tree, not _fixupChildren. This led to bugs inheriting log level as demonstrated in the new unit tests. I filed #4856 5 months ago, but never received any comments on it. Personally, I disagree with the decision to re-implement cPython's logger by calling cPython internals. In the issue I filed, I describe a reasonable alternative using exclusively the public API. But, that's a bigger project, and I just need the ability to silence ddtrace logs in my application. So for now, I left the implementation as-is and brought both the code structure and functionality in line with the cPython implementation it's imitating. ## 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/contributing.html#Release-Note-Guidelines) are followed. - I don't have all of ddtrace's tooling installed. If someone could help me generate the release notes that would be awesome. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). ## 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. --------- Co-authored-by: Zachary Groves <[email protected]> Co-authored-by: Tahir H. Butt <[email protected]> Co-authored-by: Emmett Butler <[email protected]>

brettlangdon · 2025-02-06T18:48:46Z

Finally being addressed by #12243

PGB-61 Fixes #4856 Improve and fix internal `ddtrace.internal.logger.get_logger` implementation. Today we enforce that all internal `ddtrace` loggers use a customer class implementation of `logging.Logger` to ensure we can apply custom logic like forwarding internal error logs to telemetry and rate limiting the volume of ddtrace logs allowed. However, the current implementation requires knowledge and use of internal `logging.getLogger` details, and it tries to replicate some of the behavior. This has caused us to miss a key lock around a shared resource that was expected. This change in implementation moves to apply our logic via a log filter instead of a customer logger class. A filter approach will provide the same logic with a much more simple approach which will be easier to reason and maintain which will avoid the bug/race condition found. The downside to this approach is that loggers can have multiple filters, and the first filter to determine that a log record should not be logged will prevent the other filters from running. This means if a user applies a log filter to one of our `ddtrace.*` loggers, then we may miss collecting and forwarding the internal errors to telemetry. Otherwise, if a user filter prevents a log from being logged, then it won't apply towards our rate limit (which is acceptable/correct). ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [ ] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - 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) --------- Co-authored-by: Yun Kim <[email protected]> Co-authored-by: Nicole Cybul <[email protected]> Co-authored-by: Nick Ripley <[email protected]> Co-authored-by: William Conti <[email protected]> Co-authored-by: Christophe Papazian <[email protected]> Co-authored-by: Munir Abdinur <[email protected]> Co-authored-by: Laplie Anderson <[email protected]> (cherry picked from commit 122caa6)

#12254) Backport 122caa6 from #12243 to 2.20. PGB-61 Fixes #4856 Improve and fix internal `ddtrace.internal.logger.get_logger` implementation. Today we enforce that all internal `ddtrace` loggers use a customer class implementation of `logging.Logger` to ensure we can apply custom logic like forwarding internal error logs to telemetry and rate limiting the volume of ddtrace logs allowed. However, the current implementation requires knowledge and use of internal `logging.getLogger` details, and it tries to replicate some of the behavior. This has caused us to miss a key lock around a shared resource that was expected. This change in implementation moves to apply our logic via a log filter instead of a customer logger class. A filter approach will provide the same logic with a much more simple approach which will be easier to reason and maintain which will avoid the bug/race condition found. The downside to this approach is that loggers can have multiple filters, and the first filter to determine that a log record should not be logged will prevent the other filters from running. This means if a user applies a log filter to one of our `ddtrace.*` loggers, then we may miss collecting and forwarding the internal errors to telemetry. Otherwise, if a user filter prevents a log from being logged, then it won't apply towards our rate limit (which is acceptable/correct). ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - 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) Co-authored-by: Brett Langdon <[email protected]>

PGB-61 Fixes #4856 Improve and fix internal `ddtrace.internal.logger.get_logger` implementation. Today we enforce that all internal `ddtrace` loggers use a customer class implementation of `logging.Logger` to ensure we can apply custom logic like forwarding internal error logs to telemetry and rate limiting the volume of ddtrace logs allowed. However, the current implementation requires knowledge and use of internal `logging.getLogger` details, and it tries to replicate some of the behavior. This has caused us to miss a key lock around a shared resource that was expected. This change in implementation moves to apply our logic via a log filter instead of a customer logger class. A filter approach will provide the same logic with a much more simple approach which will be easier to reason and maintain which will avoid the bug/race condition found. The downside to this approach is that loggers can have multiple filters, and the first filter to determine that a log record should not be logged will prevent the other filters from running. This means if a user applies a log filter to one of our `ddtrace.*` loggers, then we may miss collecting and forwarding the internal errors to telemetry. Otherwise, if a user filter prevents a log from being logged, then it won't apply towards our rate limit (which is acceptable/correct). ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [ ] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - 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) --------- Co-authored-by: Yun Kim <[email protected]> Co-authored-by: Nicole Cybul <[email protected]> Co-authored-by: Nick Ripley <[email protected]> Co-authored-by: William Conti <[email protected]> Co-authored-by: Christophe Papazian <[email protected]> Co-authored-by: Munir Abdinur <[email protected]> Co-authored-by: Laplie Anderson <[email protected]>

PGB-61 Fixes #4856 Improve and fix internal `ddtrace.internal.logger.get_logger` implementation. Today we enforce that all internal `ddtrace` loggers use a customer class implementation of `logging.Logger` to ensure we can apply custom logic like forwarding internal error logs to telemetry and rate limiting the volume of ddtrace logs allowed. However, the current implementation requires knowledge and use of internal `logging.getLogger` details, and it tries to replicate some of the behavior. This has caused us to miss a key lock around a shared resource that was expected. This change in implementation moves to apply our logic via a log filter instead of a customer logger class. A filter approach will provide the same logic with a much more simple approach which will be easier to reason and maintain which will avoid the bug/race condition found. The downside to this approach is that loggers can have multiple filters, and the first filter to determine that a log record should not be logged will prevent the other filters from running. This means if a user applies a log filter to one of our `ddtrace.*` loggers, then we may miss collecting and forwarding the internal errors to telemetry. Otherwise, if a user filter prevents a log from being logged, then it won't apply towards our rate limit (which is acceptable/correct). ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - 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) --------- Co-authored-by: Yun Kim <[email protected]> Co-authored-by: Nicole Cybul <[email protected]> Co-authored-by: Nick Ripley <[email protected]> Co-authored-by: William Conti <[email protected]> Co-authored-by: Christophe Papazian <[email protected]> Co-authored-by: Munir Abdinur <[email protected]> Co-authored-by: Laplie Anderson <[email protected]>

PGB-61 Fixes #4856 Improve and fix internal `ddtrace.internal.logger.get_logger` implementation. Today we enforce that all internal `ddtrace` loggers use a customer class implementation of `logging.Logger` to ensure we can apply custom logic like forwarding internal error logs to telemetry and rate limiting the volume of ddtrace logs allowed. However, the current implementation requires knowledge and use of internal `logging.getLogger` details, and it tries to replicate some of the behavior. This has caused us to miss a key lock around a shared resource that was expected. This change in implementation moves to apply our logic via a log filter instead of a customer logger class. A filter approach will provide the same logic with a much more simple approach which will be easier to reason and maintain which will avoid the bug/race condition found. The downside to this approach is that loggers can have multiple filters, and the first filter to determine that a log record should not be logged will prevent the other filters from running. This means if a user applies a log filter to one of our `ddtrace.*` loggers, then we may miss collecting and forwarding the internal errors to telemetry. Otherwise, if a user filter prevents a log from being logged, then it won't apply towards our rate limit (which is acceptable/correct). ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - 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) --------- Co-authored-by: Yun Kim <[email protected]> Co-authored-by: Nicole Cybul <[email protected]> Co-authored-by: Nick Ripley <[email protected]> Co-authored-by: William Conti <[email protected]> Co-authored-by: Christophe Papazian <[email protected]> Co-authored-by: Munir Abdinur <[email protected]> Co-authored-by: Laplie Anderson <[email protected]> (cherry picked from commit bb86a1e)

#12340) Backport bb86a1e from #12293 to 2.21. PGB-61 Fixes #4856 Improve and fix internal `ddtrace.internal.logger.get_logger` implementation. Today we enforce that all internal `ddtrace` loggers use a customer class implementation of `logging.Logger` to ensure we can apply custom logic like forwarding internal error logs to telemetry and rate limiting the volume of ddtrace logs allowed. However, the current implementation requires knowledge and use of internal `logging.getLogger` details, and it tries to replicate some of the behavior. This has caused us to miss a key lock around a shared resource that was expected. This change in implementation moves to apply our logic via a log filter instead of a customer logger class. A filter approach will provide the same logic with a much more simple approach which will be easier to reason and maintain which will avoid the bug/race condition found. The downside to this approach is that loggers can have multiple filters, and the first filter to determine that a log record should not be logged will prevent the other filters from running. This means if a user applies a log filter to one of our `ddtrace.*` loggers, then we may miss collecting and forwarding the internal errors to telemetry. Otherwise, if a user filter prevents a log from being logged, then it won't apply towards our rate limit (which is acceptable/correct). ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - 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) Co-authored-by: Brett Langdon <[email protected]>

PGB-61 Fixes #4856 Improve and fix internal `ddtrace.internal.logger.get_logger` implementation. Today we enforce that all internal `ddtrace` loggers use a customer class implementation of `logging.Logger` to ensure we can apply custom logic like forwarding internal error logs to telemetry and rate limiting the volume of ddtrace logs allowed. However, the current implementation requires knowledge and use of internal `logging.getLogger` details, and it tries to replicate some of the behavior. This has caused us to miss a key lock around a shared resource that was expected. This change in implementation moves to apply our logic via a log filter instead of a customer logger class. A filter approach will provide the same logic with a much more simple approach which will be easier to reason and maintain which will avoid the bug/race condition found. The downside to this approach is that loggers can have multiple filters, and the first filter to determine that a log record should not be logged will prevent the other filters from running. This means if a user applies a log filter to one of our `ddtrace.*` loggers, then we may miss collecting and forwarding the internal errors to telemetry. Otherwise, if a user filter prevents a log from being logged, then it won't apply towards our rate limit (which is acceptable/correct). ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [ ] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - 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) --------- Co-authored-by: Yun Kim <[email protected]> Co-authored-by: Nicole Cybul <[email protected]> Co-authored-by: Nick Ripley <[email protected]> Co-authored-by: William Conti <[email protected]> Co-authored-by: Christophe Papazian <[email protected]> Co-authored-by: Munir Abdinur <[email protected]> Co-authored-by: Laplie Anderson <[email protected]>

PGB-61 Fixes #4856 Improve and fix internal `ddtrace.internal.logger.get_logger` implementation. Today we enforce that all internal `ddtrace` loggers use a customer class implementation of `logging.Logger` to ensure we can apply custom logic like forwarding internal error logs to telemetry and rate limiting the volume of ddtrace logs allowed. However, the current implementation requires knowledge and use of internal `logging.getLogger` details, and it tries to replicate some of the behavior. This has caused us to miss a key lock around a shared resource that was expected. This change in implementation moves to apply our logic via a log filter instead of a customer logger class. A filter approach will provide the same logic with a much more simple approach which will be easier to reason and maintain which will avoid the bug/race condition found. The downside to this approach is that loggers can have multiple filters, and the first filter to determine that a log record should not be logged will prevent the other filters from running. This means if a user applies a log filter to one of our `ddtrace.*` loggers, then we may miss collecting and forwarding the internal errors to telemetry. Otherwise, if a user filter prevents a log from being logged, then it won't apply towards our rate limit (which is acceptable/correct). ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - 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) --------- Co-authored-by: Yun Kim <[email protected]> Co-authored-by: Nicole Cybul <[email protected]> Co-authored-by: Nick Ripley <[email protected]> Co-authored-by: William Conti <[email protected]> Co-authored-by: Christophe Papazian <[email protected]> Co-authored-by: Munir Abdinur <[email protected]> Co-authored-by: Laplie Anderson <[email protected]>

github-actions bot added the stale label Feb 5, 2023

mattalbr mentioned this issue Jun 8, 2023

fix(internal): call _fixupChildren when retrieving DDLogger #6080

Merged

14 tasks

github-actions bot mentioned this issue Jul 13, 2023

fix(internal): call _fixupChildren when retrieving DDLogger [backport 1.15] #6350

Merged

14 tasks

github-actions bot mentioned this issue Jul 13, 2023

fix(internal): call _fixupChildren when retrieving DDLogger [backport 1.16] #6351

Merged

14 tasks

emmettbutler closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2023

brettlangdon mentioned this issue Feb 6, 2025

fix(internal): move logging rate limiter to log filter #12243

Merged

2 tasks

brettlangdon reopened this Feb 6, 2025

github-actions bot removed the stale label Feb 7, 2025

brettlangdon closed this as completed in #12243 Feb 7, 2025

brettlangdon closed this as completed in 122caa6 Feb 7, 2025

github-actions bot mentioned this issue Feb 7, 2025

fix(internal): move logging rate limiter to log filter [backport 2.20] #12254

Merged

2 tasks

This was referenced Feb 10, 2025

fix(internal): move logging rate limiter to log filter [backport 2.x] #12264

Closed

fix(internal): move logging rate limiter to log filter #12293

Merged

github-actions bot mentioned this issue Feb 13, 2025

fix(internal): move logging rate limiter to log filter [backport 2.21] #12340

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

DDLogger doesn't respect logging hierarchy, and is not multi-process safe #4856

DDLogger doesn't respect logging hierarchy, and is not multi-process safe #4856

mattalbr commented Jan 4, 2023 •

edited

Loading

brettlangdon commented Feb 6, 2025

Uh oh!

DDLogger doesn't respect logging hierarchy, and is not multi-process safe #4856

DDLogger doesn't respect logging hierarchy, and is not multi-process safe #4856

Comments

mattalbr commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of problem

Which version of dd-trace-py are you using?

Which version of pip are you using?

Which libraries and their versions are you using?

How can we reproduce your problem?

What is the result that you get?

What is the result that you expected?

brettlangdon commented Feb 6, 2025

Uh oh!

mattalbr commented Jan 4, 2023 •

edited

Loading