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

Capture python warnings as logs #5730

Merged
merged 4 commits into from
Feb 18, 2020
Merged

Capture python warnings as logs #5730

merged 4 commits into from
Feb 18, 2020

Conversation

AlexandreYang
Copy link
Member

@AlexandreYang AlexandreYang commented Feb 13, 2020

What does this PR do?

Option 1: Capture python warnings as logs (SELECTED)

Just use logging.captureWarnings(True)

Option 2: Capture python warning as AgentCheck.warning(), so that they are visible in Datadog UI.

Would require patching warning similar to what logging does: https://github.com/python/cpython/blob/master/Lib/logging/__init__.py#L2175-L2207

This is more in line with:

Warning messages are typically issued in situations where it is useful to alert the user of some condition in a program, where that condition (normally) doesn’t warrant raising an exception and terminating the program. For example, one might want to issue a warning when a program uses an obsolete module.
https://docs.python.org/3/library/warnings.html

Motivation

Python warnings are printed to stdout which make then hard to parse for logs integration.

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@AlexandreYang AlexandreYang requested review from a team as code owners February 13, 2020 13:32
@AlexandreYang AlexandreYang changed the title Capture python warnings as log [WIP] Capture python warnings as log Feb 13, 2020
@@ -50,6 +50,8 @@

monkey_patch_pyyaml()

# Let's capture warnings as logs so it's easier for log parser to handle them.
logging.captureWarnings(True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind if we put this in the log module's init_logging?

@hithwen
Copy link
Contributor

hithwen commented Feb 13, 2020

Don't think option 2 really makes sense

@AlexandreYang
Copy link
Member Author

AlexandreYang commented Feb 13, 2020

Don't think option 2 really makes sense

@hithwen why is that ?

Isn't Python warnings conceptually more aligned with AgentCheck.warning() ?

Warning messages are typically issued in situations where it is useful to alert the user of some condition in a program, where that condition (normally) doesn’t warrant raising an exception and terminating the program. For example, one might want to issue a warning when a program uses an obsolete module.
https://docs.python.org/3/library/warnings.html

@ofek
Copy link
Contributor

ofek commented Feb 13, 2020

We need to be in control of what appears in our UI

@AlexandreYang
Copy link
Member Author

We need to be in control of what appears in our UI

That's a good point ! 👍

@codecov
Copy link

codecov bot commented Feb 18, 2020

Codecov Report

Merging #5730 into master will increase coverage by 5.96%.
The diff coverage is n/a.

Impacted Files Coverage Δ
dns_check/tests/mocks.py 96.96% <0.00%> (ø) ⬆️
eks_fargate/datadog_checks/eks_fargate/__init__.py 100.00% <0.00%> (ø) ⬆️
disk/tests/mocks.py 100.00% <0.00%> (ø) ⬆️
disk/tests/utils.py 100.00% <0.00%> (ø) ⬆️
disk/tests/common.py 63.63% <0.00%> (ø) ⬆️
disk/tests/metrics.py 100.00% <0.00%> (ø) ⬆️
apache/tests/common.py 100.00% <0.00%> (ø) ⬆️
cilium/tests/common.py 100.00% <0.00%> (ø) ⬆️
disk/tests/conftest.py 85.71% <0.00%> (ø) ⬆️
airflow/tests/common.py 100.00% <0.00%> (ø) ⬆️
... and 787 more

@AlexandreYang AlexandreYang changed the title [WIP] Capture python warnings as log Capture python warnings as log Feb 18, 2020
@AlexandreYang AlexandreYang changed the title Capture python warnings as log Capture python warnings as logS Feb 18, 2020
@AlexandreYang AlexandreYang changed the title Capture python warnings as logS Capture python warnings as logs Feb 18, 2020
@AlexandreYang AlexandreYang merged commit 32c0a0a into master Feb 18, 2020
@AlexandreYang AlexandreYang deleted the alex/capture_warnings branch February 18, 2020 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants