fix(internal): call _fixupChildren when retrieving DDLogger [backport 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]>

Author: github-actions[bot]

ddtrace/internal/logger.py

@@ -41,22 +41,26 @@ def get_logger(name):
     # DEV: This is a simplified version of `logging.Manager.getLogger`
     #   https://github.com/python/cpython/blob/48769a28ad6ef4183508951fa6a378531ace26a4/Lib/logging/__init__.py#L1221-L1253  # noqa
     # DEV: _fixupParents could be adding a placeholder, we want to replace it if that's the case
-    if name not in manager.loggerDict or isinstance(manager.loggerDict[name], logging.PlaceHolder):
-        manager.loggerDict[name] = DDLogger(name=name)
-
-    # Get our logger
-    logger = cast(DDLogger, manager.loggerDict[name])
-
-    # If this log manager has a `_fixupParents` method then call it on our logger
-    # DEV: This helper is used to ensure our logger has an appropriate `Logger.parent` set,
-    #      without this then we cannot take advantage of the root loggers handlers
-    #   https://github.com/python/cpython/blob/7c7839329c2c66d051960ab1df096aed1cc9343e/Lib/logging/__init__.py#L1272-L1294  # noqa
-    # DEV: `_fixupParents` has been around for awhile, but add the `hasattr` guard... just in case.
-    if hasattr(manager, "_fixupParents"):
-        manager._fixupParents(logger)
+    if name in manager.loggerDict:
+        logger = manager.loggerDict[name]
+        if isinstance(manager.loggerDict[name], logging.PlaceHolder):
+            placeholder = logger
+            logger = DDLogger(name=name)
+            manager.loggerDict[name] = logger
+            # DEV: `_fixupChildren` and `_fixupParents` have been around for awhile,
+            # DEV: but add the `hasattr` guard... just in case.
+            if hasattr(manager, "_fixupChildren"):
+                manager._fixupChildren(placeholder, logger)
+            if hasattr(manager, "_fixupParents"):
+                manager._fixupParents(logger)
+    else:
+        logger = DDLogger(name=name)
+        manager.loggerDict[name] = logger
+        if hasattr(manager, "_fixupParents"):
+            manager._fixupParents(logger)
 
     # Return our logger
-    return logger
+    return cast(DDLogger, logger)
 
 
 def hasHandlers(self):

releasenotes/notes/fix-logger-children-a5dddf8abbf7c159.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    internal: call ``_fixupChildren`` when retrieving ``DDLogger``

tests/tracer/test_logger.py

@@ -95,12 +95,38 @@ def test_get_logger(self):
 
         # If a PlaceHolder is in place of the logger
         # We should return the DDLogger
-        placeholder = logging.PlaceHolder("test")
-        self.manager.loggerDict["test.name.logger"] = placeholder
-        log = get_logger("test.name.logger")
-        self.assertEqual(log.name, "test.name.logger")
+        self.assertIsInstance(self.manager.loggerDict["new.test"], logging.PlaceHolder)
+        log = get_logger("new.test")
+        self.assertEqual(log.name, "new.test")
         self.assertIsInstance(log, DDLogger)
 
+    def test_get_logger_children(self):
+        """
+        When using `get_logger` to get a logger
+            We appropriately assign children loggers
+
+        DEV: This test case is to ensure we are calling `manager._fixupChildren(logger)`
+        """
+        root = get_logger("test")
+        root.setLevel(logging.WARNING)
+
+        child_logger = get_logger("test.newplaceholder.long.component")
+        self.assertEqual(child_logger.parent, root)
+
+        parent_logger = get_logger("test.newplaceholder")
+        self.assertEqual(child_logger.parent, parent_logger)
+
+        parent_logger.setLevel(logging.INFO)
+        # Because the child logger's level remains unset, it should inherit
+        # the level of its closest parent, which is INFO.
+        # If we did not properly maintain the logger tree, this would fail
+        # because child_logger would be set to the default when it was created
+        # which was logging.WARNING.
+        self.assertEqual(child_logger.getEffectiveLevel(), logging.INFO)
+
+        # Clean up for future tests.
+        root.setLevel(logging.NOTSET)
+
     def test_get_logger_parents(self):
         """
         When using `get_logger` to get a logger