-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add each checks' unique ID to logs #4410
Conversation
Codecov Report
|
Codecov Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the logic LGTM overall 👍
# the agent5 'AgentCheck' setup a log attribute. | ||
self.log = logging.getLogger('{}.{}'.format(__name__, self.name)) | ||
# We need to load this lazily because the Agent sets `check_id` after instantiation | ||
self._log = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems in this case we could use a logging.Filter
instead. Here is an example from the official cookbook:
Using Filters to impart contextual information
You can also add contextual information to log output using a user-defined Filter. Filter instances are allowed to modify the LogRecords passed to them, including adding additional attributes which can then be output using a suitable format string, or if needed a custom Formatter.
That could avoid having to create our own CheckLogger
and overriding logging methods (info/critical/warn/debug/etc), IMHO should be avoided if possible.
import logging
class ContextFilter(logging.Filter):
def __init__(self, check_id):
super(ContextFilter).__init__()
self._check_id = check_id
def filter(self, record):
record._check_id = self._check_id or "unknown"
return True
if __name__ == '__main__':
logging.basicConfig(level=logging.DEBUG,
format='%(asctime)-15s %(name)-5s %(levelname)-8s CheckID: %(_check_id)-15s %(message)s')
log = logging.getLogger('my logger')
my_check_id = 1234
log.addFilter(ContextFilter(my_check_id)) # we can also pass the check instance here if needed
log.debug('A debug message')
# output
# 2019-08-22 10:51:00,563 my logger DEBUG CheckID: 1234 A debug message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That won't work https://docs.python.org/3/library/logging.html#logging.getLogger
All calls to this function with a given name return the same logger instance.
self.log = logging.getLogger('{}.{}'.format(__name__, self.name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this by using the logging.LoggerAdapter
? :)
https://gist.github.com/AlexandreYang/c518db4380f2de53d644f17d29523ea8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[msg deleted]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, 👍
Just few comments.
This part is not critical, but few tests to check that the _check_id
is correctly transmitted could help. But up to you :)
@@ -15,6 +15,27 @@ def critical(self, msg, *args, **kwargs): | |||
raise NotImplementedError('The critical log level is reserved for agent shutdowns.') | |||
|
|||
|
|||
class CheckLoggingAdapter(logging.LoggerAdapter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we inherit from datadog_checks_base/datadog_checks/base/log.py#CheckLoggingAdapter
to reduce duplication ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No since stubs are meant to be isolated
Motivation
The ability to correlate a log entry with a specific check instance
Output