Added support for grafana loki #747
|
@ -4,6 +4,7 @@ from borgmatic.hooks import (
|
|||
cronhub,
|
||||
cronitor,
|
||||
healthchecks,
|
||||
loki,
|
||||
mariadb,
|
||||
mongodb,
|
||||
mysql,
|
||||
|
@ -11,7 +12,6 @@ from borgmatic.hooks import (
|
|||
pagerduty,
|
||||
postgresql,
|
||||
sqlite,
|
||||
loki,
|
||||
)
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
|
|
@ -1,8 +1,10 @@
|
|||
import logging
|
||||
import requests
|
||||
import json
|
||||
import time
|
||||
import logging
|
||||
import platform
|
||||
import time
|
||||
|
||||
import requests
|
||||
|
||||
from borgmatic.hooks import monitor
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
@ -16,50 +18,54 @@ MONITOR_STATE_TO_HEALTHCHECKS = {
|
|||
# Threshold at which logs get flushed to loki
|
||||
MAX_BUFFER_LINES = 100
|
||||
|
||||
class loki_log_buffer():
|
||||
|
||||
class loki_log_buffer:
|
||||
witten marked this conversation as resolved
|
||||
'''
|
||||
A log buffer that allows to output the logs as loki requests in json
|
||||
'''
|
||||
|
||||
def __init__(self, url, dry_run):
|
||||
self.url = url
|
||||
self.dry_run = dry_run
|
||||
self.root = {}
|
||||
self.root["streams"] = [{}]
|
||||
self.root["streams"][0]["stream"] = {}
|
||||
self.root["streams"][0]["values"] = []
|
||||
self.root['streams'] = [{}]
|
||||
self.root['streams'][0]['stream'] = {}
|
||||
self.root['streams'][0]['values'] = []
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
A perhaps more succinct/declarative way to express this might be:
A perhaps more succinct/declarative way to express this might be:
```
self.root = {
'streams': [{'stream': {}, 'values': []}]
}
```
|
||||
|
||||
def add_value(self, value):
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
It would be great if some of these functions have a brief docstring describing what they do. It would be great if some of these functions have a brief docstring describing what they do.
|
||||
timestamp = str(time.time_ns())
|
||||
self.root["streams"][0]["values"].append((timestamp, value))
|
||||
self.root['streams'][0]['values'].append((timestamp, value))
|
||||
|
||||
def add_label(self, label, value):
|
||||
self.root["streams"][0]["stream"][label] = value
|
||||
self.root['streams'][0]['stream'][label] = value
|
||||
|
||||
def _to_request(self):
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
Note that I've been trying to stop using underscore functions in this codebase, operating under the "we're all adults" theory of function privacy. You'll still find them in places though. Note that I've been trying to stop using underscore functions in this codebase, operating under the "we're all adults" theory of function privacy. You'll still find them in places though.
|
||||
return json.dumps(self.root)
|
||||
|
||||
def __len__(self):
|
||||
return len(self.root["streams"][0]["values"])
|
||||
return len(self.root['streams'][0]['values'])
|
||||
|
||||
def flush(self):
|
||||
if self.dry_run:
|
||||
witten marked this conversation as resolved
witten
commented
Blank lines after some of these Blank lines after some of these `if`s would be nice. Maybe before the `try` as well. In general I find that helps readability even if it's not required by the language.
|
||||
self.root["streams"][0]["values"] = []
|
||||
self.root['streams'][0]['values'] = []
|
||||
return
|
||||
if len(self) == 0:
|
||||
return
|
||||
request_body = self._to_request()
|
||||
self.root["streams"][0]["values"] = []
|
||||
request_header = {"Content-Type": "application/json"}
|
||||
self.root['streams'][0]['values'] = []
|
||||
request_header = {'Content-Type': 'application/json'}
|
||||
try:
|
||||
r = requests.post(self.url, headers=request_header, data=request_body, timeout=5)
|
||||
r.raise_for_status()
|
||||
result = requests.post(self.url, headers=request_header, data=request_body, timeout=5)
|
||||
result.raise_for_status()
|
||||
witten marked this conversation as resolved
witten
commented
This code looks good, but it's a little counter-intuitive to me that it would be here. For instance, I think of a buffer as a data structure for storing stuff, not necessarily as a data structure that also implicitly has the side effect of pushing logs to an external service. Maybe I'm just not used to OOP. 😄 I don't feel super strongly or anything, but this might be less surprising if the push to Loki took place elsewhere like in I'm guessing part of the reason you're doing it this way though is so that logs get sent to Loki as borgmatic runs rather than all at the end once Anyway, let me know your thoughts. (And then maybe put some of them into docstrings. 😄) This code looks good, but it's a little counter-intuitive to me that it would be here. For instance, I think of a buffer as a data structure for storing stuff, not necessarily as a data structure that also implicitly has the side effect of pushing logs to an external service. Maybe I'm just not used to OOP. 😄 I don't feel super strongly or anything, but this might be less surprising if the push to Loki took place elsewhere like in `ping_monitor()`.
I'm guessing part of the reason you're doing it this way though is so that logs get sent to Loki as borgmatic runs rather than all at the end once `ping_monitor()` is called..? The Healthchecks hook for example only sends logs at the end, but the rationale there is that it's explicitly logging the success/failure status of the backup rather than only logs along the way. So the requirements may be a little different.
Anyway, let me know your thoughts. (And then maybe put some of them into docstrings. 😄)
IBims1NicerTobi
commented
Well I am trying to not hit the max request size limits of a lot of loki instances that run behind e.g. nginx. I think it is much better for large volumes of logs to be pushed incrementally instead of pushing it as one who knows how big request in the end. Well I am trying to not hit the max request size limits of a lot of loki instances that run behind e.g. nginx. I think it is much better for large volumes of logs to be pushed incrementally instead of pushing it as one who knows how big request in the end.
witten
commented
Gotcha. The Healthchecks hook "solves" that particular problem by reverse truncating the logs so that older messages are not sent if the logs get too big by the time Gotcha. The Healthchecks hook "solves" that particular problem by reverse truncating the logs so that older messages are not sent if the logs get too big by the time `ping_monitor()` is called. However I can see why you might not want to do that in this case, since loki seems much more about log aggregation than simply tracking service status.
|
||||
except requests.RequestException:
|
||||
logger.warn("Failed to upload logs to loki")
|
||||
logger.warn('Failed to upload logs to loki')
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
I think this should be Also.. Nice-to-have but not required: Putting the I think this should be `.warning()` rather than `.warn()`. See [the logging docs](https://docs.python.org/3/howto/logging.html#when-to-use-logging) for more info.
Also.. Nice-to-have but not required: Putting the `config_filename` at the start of this warning message. See `healthchecks.py` error handling for an example.
|
||||
|
||||
|
||||
class loki_log_handeler(logging.Handler):
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
Spelling and capitalization: Spelling and capitalization: `Loki_log_handler`
|
||||
'''
|
||||
A log handler that sends logs to loki
|
||||
'''
|
||||
|
||||
def __init__(self, url, dry_run):
|
||||
super().__init__()
|
||||
self.buffer = loki_log_buffer(url, dry_run)
|
||||
|
@ -79,23 +85,25 @@ class loki_log_handeler(logging.Handler):
|
|||
if len(self.buffer) > 0:
|
||||
self.buffer.flush()
|
||||
|
||||
|
||||
def initialize_monitor(hook_config, config, config_filename, monitoring_log_level, dry_run):
|
||||
'''
|
||||
Add a handler to the root logger to regularly send the logs to loki
|
||||
IBims1NicerTobi marked this conversation as resolved
witten
commented
Just a code style convention nit: Period at the end of sentences in docstrings. Just a code style convention nit: Period at the end of sentences in docstrings.
|
||||
'''
|
||||
url = hook_config.get('url')
|
||||
loki = loki_log_handeler(url, dry_run)
|
||||
for k, v in hook_config.get('labels').items():
|
||||
if v == '__hostname':
|
||||
loki.add_label(k, platform.node())
|
||||
elif v == '__config':
|
||||
loki.add_label(k, config_filename.split('/')[-1])
|
||||
elif v == '__config_path':
|
||||
loki.add_label(k, config_filename)
|
||||
for key, value in hook_config.get('labels').items():
|
||||
if value == '__hostname':
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
Will users expect to substitute these placeholders within longer strings? Example value: Will users expect to substitute these placeholders within longer strings? Example value: `The name of this host is __hostname`
IBims1NicerTobi
commented
Not really. Grafana loki labels are rarely longer than a single word and the placeholders can be compared to what grafana themself have implemented in promtail which is their loki log agent. The label format is taken from https://grafana.com/docs/loki/latest/clients/promtail/configuration/#syslog. It works the same for almost all of promtail and I think I would rather adhere to their ecosystem of how they label streams than this one but thats up for debate. Not really. Grafana loki labels are rarely longer than a single word and the placeholders can be compared to what grafana themself have implemented in promtail which is their loki log agent. The label format is taken from https://grafana.com/docs/loki/latest/clients/promtail/configuration/#syslog. It works the same for almost all of promtail and I think I would rather adhere to their ecosystem of how they label streams than this one but thats up for debate.
witten
commented
I'm fine leaving it as-is if there's already convention on the loki side. I'm fine leaving it as-is if there's already convention on the loki side.
|
||||
loki.add_label(key, platform.node())
|
||||
elif value == '__config':
|
||||
loki.add_label(key, config_filename.split('/')[-1])
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
Check out Check out `os.path.basename()` for this!
IBims1NicerTobi
commented
I wanted to ask about this anyway: Is the hostname of the system used anywhere else so we can have consistent naming? Not that it matters much for this as users can change the hostname label anyways but it would still be good to be consistent I wanted to ask about this anyway: Is the hostname of the system used anywhere else so we can have consistent naming? Not that it matters much for this as users can change the hostname label anyways but it would still be good to be consistent
witten
commented
In borgmatic code, I think the PagerDuty hook uses hostname, but that's about it. Well, I guess hostname can be part of the In borgmatic code, I think the PagerDuty hook uses hostname, but that's about it. Well, I guess hostname can be part of the `archive_name_format` configured in borgmatic but then it's passed to Borg.
|
||||
elif value == '__config_path':
|
||||
loki.add_label(key, config_filename)
|
||||
else:
|
||||
loki.add_label(k, v)
|
||||
loki.add_label(key, value)
|
||||
logging.getLogger().addHandler(loki)
|
||||
|
||||
|
||||
def ping_monitor(hook_config, config, config_filename, state, monitoring_log_level, dry_run):
|
||||
'''
|
||||
Adds an entry to the loki logger with the current state
|
||||
|
@ -106,6 +114,7 @@ def ping_monitor(hook_config, config, config_filename, state, monitoring_log_lev
|
|||
if state in MONITOR_STATE_TO_HEALTHCHECKS.keys():
|
||||
handler.raw(f'{config_filename} {MONITOR_STATE_TO_HEALTHCHECKS[state]} backup')
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
Maybe put a Is there a reason though you're picking out the loki log handler to log to instead of just logging this as a general log message? E.g., you could just throw the word "loki" somewhere in it and use Maybe put a `:` after `{config_filename}` as per convention.
Is there a reason though you're picking out the loki log handler to log to instead of just logging this as a general log message? E.g., you could just throw the word "loki" somewhere in it and use `logger.info()` or `.debug()` or whatever. Again, check out the other monitoring hooks for examples.
IBims1NicerTobi
commented
Yes there is a reason for doing it this way. First of all as far as I am aware the function is just supposed to inform the monitoring app of the current state. If we used Yes there is a reason for doing it this way. First of all as far as I am aware the function is just supposed to inform the monitoring app of the current state. If we used `logger.info()` the message will depend on the current log level and settings etc. Skipping the logging system makes sure that: 1. The message is only send to loki and not any other agents (which should generate their own message) 2. The message is always send regardless of how the application logger is configured.
witten
commented
Okay, gotcha. Okay, gotcha.
|
||||
|
||||
|
||||
def destroy_monitor(hook_config, config, config_filename, monitoring_log_level, dry_run):
|
||||
'''
|
||||
Remove the monitor handler that was added to the root logger.
|
||||
|
|
Code style convention: Uppercase first letter of class names, e.g.
Loki_log_buffer
.