[Bug] Accidentally attaching adapters to root logger leads to increased logging costs

[glebkrapivin]

Closes #1048

Description

After having upgraded to astronomer-cosmos==1.4.3 we noticed an unusual 2x increase of the cost of the logs in our cloud provider. Quick investigation showed that the logs from cosmos logger were duplicated multiple times.
Example:

[2024-06-17 12:18:48,259] {{manager.py:165}} INFO - Launched DagFileProcessorManager with pid: 656
2024-06-17T12:18:48.260107549Z [2024-06-17 12:18:48,259] {manager.py:165} INFO - (astronomer-cosmos) - Launched DagFileProcessorManager with pid: 656
2024-06-17T12:18:48.260225215Z [2024-06-17 12:18:48,259] {manager.py:165} INFO - (astronomer-cosmos) - Launched DagFileProcessorManager with pid: 656
2024-06-17T12:18:48.260345799Z [2024-06-17 12:18:48,259] {manager.py:165} INFO - (astronomer-cosmos) - Launched DagFileProcessorManager with pid: 656

After reproducing the problem locally I've found the highest version that seemed to not have the problem, which was 1.3.2. However, the problem repeated itself when cosmos was imported in the dag. Afterwards, I looked at the changed files and saw that airflow plugin was added in the 1.4.0 version. It explains why the issue reproduces in all the version, but is hidden at first in 1.3.2

[project.entry-points."airflow.plugins"]
cosmos = "cosmos.plugin:CosmosPlugin"

Following this discovery i looked through the source code and saw that in some of the lines the logger was initialized though function get_logger without the name argument. If you call logging.getLogger(None), you get a root logger. Hence, cosmos adapter got attached to root logger multiple times in these places.

def get_logger(name: str) -> logging.Logger:
    logger = logging.getLogger(None) # Now it is a root logger
    ....
    logger.addHandler(handler) # now we have added astronomer logger to root logger, which is not intented

What was done

• I made the name argument in get_logger required, otherwise it would be easy to forget to add it when a new file with logger call is created.
• Added LOGGER_NAME_TEMPLATE so that there are no collisions with the adapters attaching to wrong loggers in the future. The name is not used in the actual format of the log message, so it does not matter much.

The previous tests did not catch this corner case, but when the argument is required there is no need to check it

Related Issue(s)

No

Breaking Change?

No

Checklist

• I have made corresponding changes to the documentation (if required)
• I have added tests that prove my fix is effective or that my feature works

[netlify]

✅ Deploy Preview for sunny-pastelito-5ecb04 ready!

Name	Link
🔨 Latest commit	3311c80
🔍 Latest deploy log	https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/66716af95eec5f00089ec388
😎 Deploy Preview	https://deploy-preview-1047--sunny-pastelito-5ecb04.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[glebkrapivin]

@tatiana

I've provided the solution as well. Would appreciate if we could prioritize this small bugfix as this is connected to costs. Moreover, other guys might have that as well.

[tatiana]

Hi @glebkrapivin , thanks for raising the issue, identifying the problem, and proposing a solution. This is a great work!
Would there be a way for us to add some tests to make sure we avoid this same problem in the future? Perhaps if you could add a unit test on get_logger without a name and check that it raises an exception, it would be better than no tests.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.81%. Comparing base (e4f9ece) to head (3311c80).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1047   +/-   ##
=======================================
  Coverage   95.81%   95.81%           
=======================================
  Files          62       62           
  Lines        3009     3010    +1     
=======================================
+ Hits         2883     2884    +1     
  Misses        126      126           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.