Skip to content
This repository was archived by the owner on Sep 6, 2025. It is now read-only.

Several enhancements to server status checking, logging and configurability#7

Open
pataquets wants to merge 4 commits into
hellofresh:masterfrom
pataquets:improvements--server-status--configuration--logging
Open

Several enhancements to server status checking, logging and configurability#7
pataquets wants to merge 4 commits into
hellofresh:masterfrom
pataquets:improvements--server-status--configuration--logging

Conversation

@pataquets
Copy link
Copy Markdown
Contributor

Thanks for sharing this useful tool.
Based on top of #6, which I used as my dev workflow, I've done some improvements which will be useful in general and enhance running inside Docker containers.
A few more to come later.

  • Checker:
    • Handle 'SHOW STATUS' query as a dictionary and access returned values by key.
    • Move values logging under each variable, to improve traceability in case any of them is not available and execution breaks.
    • Now 'lag_interval' and 'lag_duration' values are read from config keys.
    • Add replication status row debug logging.
    • Minor message fixes.
  • General:
    • Some already existing config keys now are optional, with previous values as defaults.
    • Logging is now also configurable: log level and log destination (file or console).
    • Added a few debug-level logging here and there to help at troubleshooting.
    • Config template updated as needed, adding some help.

@pataquets
Copy link
Copy Markdown
Contributor Author

pataquets commented Jan 26, 2018

Also, for the time being, I've kept everything backwards compatible to avoid breaking current setups.
But I would like to suggest the following defaults to be changed:

  • Log level to be INFO.
  • Logging to stderr.

But not required, whatsoever.

@pataquets
Copy link
Copy Markdown
Contributor Author

After this and #6 land, I'll do the latest remaining enhancements to enable full, seamless Docker usage, while fully keeping compatibility.

…bility:

- Checker:
-- Handle 'SHOW STATUS' query as a dictionary and access returned values by key.
-- Move values logging under each variable, to improve traceability in case any of them is not available and execution breaks.
-- Now 'lag_interval' and 'lag_duration' values are read from config keys.
-- Add replication status row debug logging.
-- Minor message fixes.
- General:
-- Some already existing config keys now are optional, with previous values as defaults.
-- Logging is now also configurable: log level and log destination (file or console).
-- Added a few debug-level logging here and there to help at troubleshooting.
-- Config template updated as needed, adding some help.
@pataquets pataquets force-pushed the improvements--server-status--configuration--logging branch from ffc9ccb to bd3bed7 Compare January 26, 2018 19:58
@pataquets
Copy link
Copy Markdown
Contributor Author

Ping @mandoz , since the repo owner is an organization, and in case the notification went unnoticed.

Comment thread checkers/replication.py
import logging

from pprint import pformat

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing new line before class name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍 I thought it was better to keep imports in the same block.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes it is, you need two empty lines between the last import and the class declaration.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ops! Didn't knew it. Thanks for the reference link.

Comment thread config.yml.template
#loglevel: debug

# Use special filename 'stderr' to log to console instead of logging to file.
#logfile: replication.log
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Better to configure log path in my opinion that just log file name. For example a user might want to log to /var/log

Copy link
Copy Markdown
Contributor Author

@pataquets pataquets Feb 16, 2018

Choose a reason for hiding this comment

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

@mandoz You mean just in the sample config value? Just change replication.log to /var/log/replication.log?
Some help text additional clarification?
Or something in the feature code/functionality?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mandoz : ping?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mandoz : Any feedback on this? TIA

Comment thread config.yml.template Outdated
# Use special filename 'stderr' to log to console instead of logging to file.
#logfile: replication.log

#webhook_url: 'https://hooks.slack.com/services/XXXXX/XXXXX/XXXXX' No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a required config for slack notifications. Why is it commented out?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍 Ops! Done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants