-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🎉 CDK: use standard logger with custom handler #6085
Conversation
logging.Logger.__init__(self, name) | ||
self.valid_log_types = {"FATAL": 50, "ERROR": 40, "WARN": 30, "INFO": 20, "DEBUG": 10, "TRACE": 5} | ||
|
||
def log_by_prefix(self, msg, default_level): |
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.
add docstrings please
airbyte-cdk/python/CHANGELOG.md
Outdated
@@ -1,5 +1,8 @@ | |||
# Changelog | |||
|
|||
## 0.1.19 | |||
Use standard logger with custom handler |
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.
Use standard logger with custom handler | |
Use python standard logging instead of custom class |
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.
can we have simple test for this?
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.
Agree with @keu comments
I need advice how to better test logging. |
def get_logger(): | ||
logging.setLoggerClass(AirbyteNativeLogger) | ||
logging.addLevelName(TRACE_LEVEL_NUM, "TRACE") | ||
logger = logging.getLogger("airbyte.source.zabbix") |
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.
again, why did you hardcode this 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.
also, you need to set up this for root-level logger only. We are not particularly interested in return result, this will configure global object, so all loggers derived from root will follow this configuration, i.e.
logging.info(...)
will work as intended
@@ -45,7 +45,7 @@ class Stream(ABC): | |||
""" | |||
|
|||
# Use self.logger in subclasses to log any messages | |||
logger = AirbyteLogger() # TODO use native "logging" loggers with custom handlers | |||
logger = get_logger() |
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.
logger = get_logger() | |
@property | |
def logger(self): | |
return logging.getLogger(f'streams.{self.name}') |
logging.setLoggerClass(AirbyteNativeLogger) | ||
logging.addLevelName(TRACE_LEVEL_NUM, "TRACE") | ||
logger = logging.getLogger("airbyte.source.zabbix") | ||
logger.setLevel(TRACE_LEVEL_NUM) |
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.
also please use dict like configuration
LOGGING_CONFIG = {
'version': 1,
'disable_existing_loggers': False,
'formatters': {
'airbyte': {
'class': 'AirbyteLogFormatter,
'format': '%(message)s'
},
},
'handlers': {
'console': {
'class': 'logging.StreamHandler',
'formatter': 'simple',
'level': 'TRACE'
},
},
'root': {
'handlers': ['console'],
'level': 'TRACE'
}
}
logging.config.dictConfig(config)
return logger | ||
|
||
|
||
class AirbyteLogFormatter(logging.Formatter): |
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.
docstrings
|
||
|
||
class AirbyteLogFormatter(logging.Formatter): | ||
def format(self, record): |
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.
annotations
|
||
|
||
class AirbyteNativeLogger(logging.Logger): | ||
def __init__(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.
docstrings explaining why do we this class would be nice
|
||
class AirbyteNativeLogger(logging.Logger): | ||
def __init__(self, name): | ||
logging.Logger.__init__(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.
logging.Logger.__init__(self, name) | |
super().__init__(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.
see comments
@vitaliizazmic to test logging try to mock stdout, init logging and log different messages with different levels |
|
||
class AirbyteEntrypoint(object): | ||
def __init__(self, source: Source): | ||
self.source = source | ||
self.logger = init_logger(source.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.
nice!
# Conflicts: # airbyte-cdk/python/CHANGELOG.md # airbyte-cdk/python/setup.py
What
Use standard logger with custom handler
Closes #1279
How
Create according to issue AirbyteLogFormatter and AirbyteNativeLogger, which inherits logging.Logger.