Added support for grafana loki #747
|
@ -1403,3 +1403,33 @@ properties:
|
|||
Configuration for a monitoring integration with Crunhub. Create an
|
||||
account at https://cronhub.io if you'd like to use this service. See
|
||||
borgmatic monitoring documentation for details.
|
||||
loki:
|
||||
type: object
|
||||
required: ['url', 'labels']
|
||||
additionalProperties: false
|
||||
properties:
|
||||
url:
|
||||
type: string
|
||||
description: |
|
||||
Grafana loki log URL to notify when a backup begins,
|
||||
ends, or fails.
|
||||
example: "http://localhost:3100/loki/api/v1/push"
|
||||
labels:
|
||||
type: object
|
||||
additionalProperties:
|
||||
type: string
|
||||
witten marked this conversation as resolved
|
||||
description: |
|
||||
Allows setting custom labels for the logging stream. At
|
||||
least one label is required. "__hostname" gets replaced by
|
||||
the machine hostname automatically. "__config" gets replaced
|
||||
by just the name of the configuration file. "__config_path"
|
||||
witten marked this conversation as resolved
witten
commented
What do you think of standardizing on the What do you think of standardizing on the `{name}` placeholder syntax that's used elsewhere in borgmatic's configuration file and also by Borg as well?
|
||||
gets replaced by the full path of the configuration file.
|
||||
example:
|
||||
app: "borgmatic"
|
||||
config: "__config"
|
||||
hostname: "__hostname"
|
||||
description: |
|
||||
Configuration for a monitoring integration with Grafana loki. You
|
||||
can send the logs to a self-hosted instance or create an account at
|
||||
https://grafana.com/auth/sign-up/create-user. See borgmatic
|
||||
monitoring documentation for details.
|
||||
|
|
|
@ -4,6 +4,7 @@ from borgmatic.hooks import (
|
|||
cronhub,
|
||||
cronitor,
|
||||
healthchecks,
|
||||
loki,
|
||||
mariadb,
|
||||
mongodb,
|
||||
mysql,
|
||||
|
@ -26,6 +27,7 @@ HOOK_NAME_TO_MODULE = {
|
|||
'pagerduty': pagerduty,
|
||||
'postgresql_databases': postgresql,
|
||||
'sqlite_databases': sqlite,
|
||||
'loki': loki,
|
||||
}
|
||||
|
||||
|
||||
|
|
|
@ -0,0 +1,149 @@
|
|||
import json
|
||||
import logging
|
||||
import os
|
||||
import platform
|
||||
import time
|
||||
|
||||
import requests
|
||||
|
||||
from borgmatic.hooks import monitor
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
witten marked this conversation as resolved
witten
commented
This should probably be renamed to This should probably be renamed to `MONITOR_STATE_TO_LOKI`. 😄
|
||||
MONITOR_STATE_TO_LOKI = {
|
||||
monitor.State.START: 'Started',
|
||||
monitor.State.FINISH: 'Finished',
|
||||
monitor.State.FAIL: 'Failed',
|
||||
}
|
||||
|
||||
# Threshold at which logs get flushed to loki
|
||||
MAX_BUFFER_LINES = 100
|
||||
|
||||
|
||||
witten marked this conversation as resolved
witten
commented
Code style convention: Uppercase first letter of class names, e.g. Code style convention: Uppercase first letter of class names, e.g. `Loki_log_buffer`.
|
||||
class Loki_log_buffer:
|
||||
'''
|
||||
A log buffer that allows to output the logs as loki requests in json. Allows
|
||||
adding labels to the log stream and takes care of communication with loki.
|
||||
'''
|
||||
|
||||
def __init__(self, url, dry_run):
|
||||
self.url = url
|
||||
self.dry_run = dry_run
|
||||
self.root = {'streams': [{'stream': {}, '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.
|
||||
Add a log entry to the stream.
|
||||
'''
|
||||
timestamp = str(time.time_ns())
|
||||
self.root['streams'][0]['values'].append((timestamp, value))
|
||||
|
||||
def add_label(self, label, value):
|
||||
'''
|
||||
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.
|
||||
Add a label to the logging stream.
|
||||
'''
|
||||
self.root['streams'][0]['stream'][label] = value
|
||||
|
||||
def to_request(self):
|
||||
return json.dumps(self.root)
|
||||
|
||||
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.
|
||||
def __len__(self):
|
||||
'''
|
||||
Gets the number of lines currently in the buffer.
|
||||
'''
|
||||
return len(self.root['streams'][0]['values'])
|
||||
|
||||
def flush(self):
|
||||
if self.dry_run:
|
||||
# Just empty the buffer and skip
|
||||
self.root['streams'][0]['values'] = []
|
||||
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.
|
||||
logger.info('Skipped uploading logs to loki due to dry run')
|
||||
return
|
||||
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.
|
||||
|
||||
if len(self) == 0:
|
||||
# Skip as there are not logs to send yet
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
Spelling and capitalization: Spelling and capitalization: `Loki_log_handler`
|
||||
return
|
||||
|
||||
request_body = self.to_request()
|
||||
self.root['streams'][0]['values'] = []
|
||||
request_header = {'Content-Type': 'application/json'}
|
||||
try:
|
||||
result = requests.post(self.url, headers=request_header, data=request_body, timeout=5)
|
||||
result.raise_for_status()
|
||||
except requests.RequestException:
|
||||
logger.warning('Failed to upload logs to loki')
|
||||
|
||||
|
||||
class Loki_log_handler(logging.Handler):
|
||||
'''
|
||||
A log handler that sends logs to loki.
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
Some brief docstrings would be good on these functions too. For instance, I'm not sure at a glance why this function is called Some brief docstrings would be good on these functions too. For instance, I'm not sure at a glance why this function is called `.raw()`.. I'm sure there's a good reason though.
|
||||
'''
|
||||
|
||||
def __init__(self, url, dry_run):
|
||||
super().__init__()
|
||||
self.buffer = Loki_log_buffer(url, dry_run)
|
||||
|
||||
def emit(self, record):
|
||||
'''
|
||||
Add a log record from the logging module to the stream.
|
||||
'''
|
||||
self.raw(record.getMessage())
|
||||
|
||||
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.
|
||||
def add_label(self, key, value):
|
||||
'''
|
||||
Add a label to the logging stream.
|
||||
'''
|
||||
self.buffer.add_label(key, value)
|
||||
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.
|
||||
|
||||
def raw(self, msg):
|
||||
'''
|
||||
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.
|
||||
Add an arbitrary string as a log entry to the stream.
|
||||
'''
|
||||
self.buffer.add_value(msg)
|
||||
if len(self.buffer) > MAX_BUFFER_LINES:
|
||||
self.buffer.flush()
|
||||
|
||||
def flush(self):
|
||||
'''
|
||||
Send the logs to loki and empty the buffer.
|
||||
'''
|
||||
self.buffer.flush()
|
||||
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
You could instead do:
... just to cut down on the deep indentation a bit. That would also give you an opportunity to log about the fact that it's a dry run and you're bailing. See You could instead do:
```
if dry_run:
return
```
... just to cut down on the deep indentation a bit. That would also give you an opportunity to log about the fact that it's a dry run and you're bailing. See `healthchecks.py` or one of the other monitoring hooks for an example.
IBims1NicerTobi
commented
I don't think adding a entry that we are bailing is useful here as I added the entry for every time we flush the buffer now (which is at least once per program). I don't think adding a entry that we are bailing is useful here as I added the entry for every time we flush the buffer now (which is at least once per program).
witten
commented
Sounds good. Sounds good.
|
||||
|
||||
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.
|
||||
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.
|
||||
'''
|
||||
url = hook_config.get('url')
|
||||
loki = Loki_log_handler(url, dry_run)
|
||||
for key, value in hook_config.get('labels').items():
|
||||
if value == '__hostname':
|
||||
loki.add_label(key, platform.node())
|
||||
elif value == '__config':
|
||||
loki.add_label(key, os.path.basename(config_filename))
|
||||
elif value == '__config_path':
|
||||
loki.add_label(key, config_filename)
|
||||
else:
|
||||
loki.add_label(key, value)
|
||||
logging.getLogger().addHandler(loki)
|
||||
|
||||
|
||||
def ping_monitor(hook_config, config, config_filename, state, monitoring_log_level, dry_run):
|
||||
'''
|
||||
Add an entry to the loki logger with the current state.
|
||||
'''
|
||||
for handler in tuple(logging.getLogger().handlers):
|
||||
if isinstance(handler, Loki_log_handler):
|
||||
if state in MONITOR_STATE_TO_LOKI.keys():
|
||||
handler.raw(f'{config_filename}: {MONITOR_STATE_TO_LOKI[state]} backup')
|
||||
|
||||
|
||||
def destroy_monitor(hook_config, config, config_filename, monitoring_log_level, dry_run):
|
||||
'''
|
||||
Remove the monitor handler that was added to the root logger.
|
||||
'''
|
||||
logger = logging.getLogger()
|
||||
for handler in tuple(logger.handlers):
|
||||
if isinstance(handler, Loki_log_handler):
|
||||
handler.flush()
|
||||
logger.removeHandler(handler)
|
|
@ -1,6 +1,6 @@
|
|||
from enum import Enum
|
||||
|
||||
MONITOR_HOOK_NAMES = ('healthchecks', 'cronitor', 'cronhub', 'pagerduty', 'ntfy')
|
||||
MONITOR_HOOK_NAMES = ('healthchecks', 'cronitor', 'cronhub', 'pagerduty', 'ntfy', 'loki')
|
||||
|
||||
|
||||
class State(Enum):
|
||||
|
|
|
@ -0,0 +1,82 @@
|
|||
import logging
|
||||
import platform
|
||||
|
||||
from flexmock import flexmock
|
||||
|
||||
from borgmatic.hooks import loki as module
|
||||
|
||||
|
||||
def test_log_handler_label_replacment():
|
||||
'''
|
||||
Assert that label placeholders get replaced
|
||||
'''
|
||||
hook_config = {
|
||||
'url': 'http://localhost:3100/loki/api/v1/push',
|
||||
'labels': {'hostname': '__hostname', 'config': '__config', 'config_full': '__config_path'},
|
||||
}
|
||||
config_filename = '/mock/path/test.yaml'
|
||||
dry_run = True
|
||||
module.initialize_monitor(hook_config, flexmock(), config_filename, flexmock(), dry_run)
|
||||
for handler in tuple(logging.getLogger().handlers):
|
||||
if isinstance(handler, module.Loki_log_handler):
|
||||
assert handler.buffer.root['streams'][0]['stream']['hostname'] == platform.node()
|
||||
assert handler.buffer.root['streams'][0]['stream']['config'] == 'test.yaml'
|
||||
assert handler.buffer.root['streams'][0]['stream']['config_full'] == config_filename
|
||||
return
|
||||
assert False
|
||||
|
||||
|
||||
def test_initalize_adds_log_handler():
|
||||
'''
|
||||
Assert that calling initialize_monitor adds our logger to the root logger
|
||||
'''
|
||||
hook_config = {'url': 'http://localhost:3100/loki/api/v1/push', 'labels': {'app': 'borgmatic'}}
|
||||
module.initialize_monitor(
|
||||
hook_config,
|
||||
flexmock(),
|
||||
config_filename='test.yaml',
|
||||
monitoring_log_level=flexmock(),
|
||||
dry_run=True,
|
||||
)
|
||||
for handler in tuple(logging.getLogger().handlers):
|
||||
if isinstance(handler, module.Loki_log_handler):
|
||||
return
|
||||
assert False
|
||||
|
||||
|
||||
def test_ping_adds_log_message():
|
||||
'''
|
||||
Assert that calling ping_monitor adds a message to our logger
|
||||
'''
|
||||
hook_config = {'url': 'http://localhost:3100/loki/api/v1/push', 'labels': {'app': 'borgmatic'}}
|
||||
config_filename = 'test.yaml'
|
||||
dry_run = True
|
||||
module.initialize_monitor(hook_config, flexmock(), config_filename, flexmock(), dry_run)
|
||||
module.ping_monitor(
|
||||
hook_config, flexmock(), config_filename, module.monitor.State.FINISH, flexmock(), dry_run
|
||||
)
|
||||
for handler in tuple(logging.getLogger().handlers):
|
||||
if isinstance(handler, module.Loki_log_handler):
|
||||
assert any(
|
||||
map(
|
||||
lambda log: log
|
||||
== f'{config_filename}: {module.MONITOR_STATE_TO_LOKI[module.monitor.State.FINISH]} backup',
|
||||
map(lambda x: x[1], handler.buffer.root['streams'][0]['values']),
|
||||
)
|
||||
)
|
||||
return
|
||||
assert False
|
||||
|
||||
|
||||
def test_log_handler_gets_removed():
|
||||
'''
|
||||
Assert that destroy_monitor removes the logger from the root logger
|
||||
'''
|
||||
hook_config = {'url': 'http://localhost:3100/loki/api/v1/push', 'labels': {'app': 'borgmatic'}}
|
||||
config_filename = 'test.yaml'
|
||||
dry_run = True
|
||||
module.initialize_monitor(hook_config, flexmock(), config_filename, flexmock(), dry_run)
|
||||
module.destroy_monitor(hook_config, flexmock(), config_filename, flexmock(), dry_run)
|
||||
for handler in tuple(logging.getLogger().handlers):
|
||||
if isinstance(handler, module.Loki_log_handler):
|
||||
assert False
|
|
@ -0,0 +1,98 @@
|
|||
import json
|
||||
|
||||
import requests
|
||||
from flexmock import flexmock
|
||||
|
||||
from borgmatic.hooks import loki as module
|
||||
|
||||
witten marked this conversation as resolved
witten
commented
There's a convention in this codebase to import the module under test as
... and then refer to it as such in the tests. This makes is super clear what's part of the unit under test and what isn't. There's a convention in this codebase to import the module under test as `module`. E.g.:
```python
from borgmatic.hooks import loki as module
```
... and then refer to it as such in the tests. This makes is super clear what's part of the unit under test and what isn't.
IBims1NicerTobi
commented
Thats fixed now Thats fixed now
|
||||
|
||||
def test_log_handler_gets_labels():
|
||||
witten marked this conversation as resolved
witten
commented
Test function naming convention in this codebase is to include the name of the unit under test after Similar for other test functions in this file. Test function naming convention in this codebase is to include the name of the unit under test after `test_`. So for instance you could call this `test_initialize_monitor_adds_log_handler()`. IMO this makes it easier to find relevant tests and forces you to pick a single unit to really focus on in each test.
Similar for other test functions in this file.
witten
commented
I'll go ahead and do this after merging rather than having to do another round of back-and-forth on the PR! I'll go ahead and do this after merging rather than having to do another round of back-and-forth on the PR!
|
||||
'''
|
||||
Assert that adding labels works
|
||||
'''
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
You could move these inline to the function call as keyword arguments ( You could move these inline to the function call as keyword arguments (`... , config_filename='test.yaml', ...`) since they're only used once.
|
||||
buffer = module.Loki_log_buffer(flexmock(), False)
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
Instead of Same thing elsewhere in this file. Instead of `''` as placeholder for values that don't matter, the convention here is to use `flexmock()` instances (which you can by `from flexmock import flexmock`). That has the benefit of giving more sensible error messages if they do end up used.
Same thing elsewhere in this file.
|
||||
buffer.add_label('test', 'label')
|
||||
assert buffer.root['streams'][0]['stream']['test'] == 'label'
|
||||
buffer.add_label('test2', 'label2')
|
||||
witten marked this conversation as resolved
witten
commented
You could safely delete this You could safely delete this `assert` since it'll always be true.
|
||||
assert buffer.root['streams'][0]['stream']['test2'] == 'label2'
|
||||
|
||||
witten marked this conversation as resolved
witten
commented
If you wanted to make this loop and assertion a little more declarative, you could do:
Do not feel strongly. If you wanted to make this loop and assertion a little more declarative, you could do:
```
assert any(
isinstance(handler, loki.Loki_log_handler)
for handler in tuple(logging.getLogger().handlers)
)
```
Do not feel strongly.
|
||||
|
||||
witten marked this conversation as resolved
witten
commented
So looking at the preceding test, I don't think it actually qualifies as a true "unit" test in that its unit under test ( This may seem like unnecessary pedantry, but this sort of rigor is intended to make it super clear what sort of coverage a given unit has—and make sure units actually get unit tested where that makes sense. Also: IMO you don't need to mock out So looking at the preceding test, I don't think it actually qualifies as a true "unit" test in that its unit under test (`initialize_monitor()`) integrates with another unit (`Loki_log_handler`). So I think your options here are: 1. Mock out `Loki_log_handler` with `flexmock` or 2. Move this test into the `integration` test directory.
This may seem like unnecessary pedantry, but this sort of rigor is intended to make it super clear what sort of coverage a given unit has—and make sure units actually get unit tested where that makes sense.
Also: IMO you don't need to mock out `platform.node()` or `os.path.basename()` in this particular test though, because those code paths aren't triggered here. And I generally don't mock out Python's logging system in a test because it's standard library.
|
||||
def test_log_buffer_gets_raw():
|
||||
'''
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
Besides the name of the unit under test, it would be great to have your expectation encoded into the test name. Example: Besides the name of the unit under test, it would be great to have your expectation encoded into the test name. Example: `test_ping_monitor_flushes_buffer()` if that's indeed what you're expecting to happen.
|
||||
Assert that adding values to the log buffer increases it's length
|
||||
'''
|
||||
buffer = module.Loki_log_buffer(flexmock(), False)
|
||||
assert len(buffer) == 0
|
||||
buffer.add_value('Some test log line')
|
||||
assert len(buffer) == 1
|
||||
buffer.add_value('Another test log line')
|
||||
assert len(buffer) == 2
|
||||
witten marked this conversation as resolved
witten
commented
Interesting. I think what you're doing here is checking that the buffer actually got flushed, which seems like a reasonable thing to do in this test. But one failure mode I can see is if nothing got put into the buffer to begin with. Is there some way to prevent that at the test level? Maybe that case is covered by the other tests below? I'll keep reading! In any case, why Interesting. I think what you're doing here is checking that the buffer actually got flushed, which seems like a reasonable thing to do in this test. But one failure mode I can see is if nothing got put into the buffer to begin with. Is there some way to prevent that at the test level? Maybe that case is covered by the other tests below? I'll keep reading!
In any case, why `<= 1` instead of `== 0`?
IBims1NicerTobi
commented
I changed this test to makes more sense. It now checks that the buffer gets the exact message I expect. I changed this test to makes more sense. It now checks that the buffer gets the exact message I expect.
|
||||
|
||||
|
||||
witten marked this conversation as resolved
witten
commented
Similar feedback for this test: I think it either needs mocking to make it into a unit test.. or you can move it into Similar feedback for this test: I think it either needs mocking to make it into a unit test.. or you can move it into `integration` if you prefer to keep it integrating multiple functions.
|
||||
def test_log_buffer_gets_log_messages():
|
||||
'''
|
||||
Assert that adding log records works
|
||||
'''
|
||||
handler = module.Loki_log_handler(flexmock(), False)
|
||||
handler.emit(flexmock(getMessage=lambda: 'Some test log line'))
|
||||
assert len(handler.buffer) == 1
|
||||
|
||||
|
||||
def test_log_buffer_json():
|
||||
'''
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
You could use the You could use the `any()` idiom here too.
|
||||
Assert that the buffer correctly serializes when empty
|
||||
'''
|
||||
buffer = module.Loki_log_buffer(flexmock(), False)
|
||||
assert json.loads(buffer.to_request()) == json.loads('{"streams":[{"stream":{},"values":[]}]}')
|
||||
|
||||
|
||||
def test_log_buffer_json_labels():
|
||||
'''
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
For this
For this `assert` (and maybe the previous one) it'd be good to assert the entire `'stream'` structure IMO. That way, you're ensuring that adding one label doesn't erroneously replace previous ones. Example:
```python
assert buffer.root['streams'][0]['stream'] == {
'test': 'label',
'test2': 'label2',
}
```
|
||||
Assert that the buffer correctly serializes with labels
|
||||
'''
|
||||
buffer = module.Loki_log_buffer(flexmock(), False)
|
||||
buffer.add_label('test', 'label')
|
||||
assert json.loads(buffer.to_request()) == json.loads(
|
||||
'{"streams":[{"stream":{"test": "label"},"values":[]}]}'
|
||||
)
|
||||
|
||||
|
||||
def test_log_buffer_json_log_lines():
|
||||
'''
|
||||
Assert that log lines end up in the correct place in the log buffer
|
||||
'''
|
||||
buffer = module.Loki_log_buffer(flexmock(), False)
|
||||
buffer.add_value('Some test log line')
|
||||
witten marked this conversation as resolved
witten
commented
You could also do a single full- You could also do a single full-`'stream'` assert here. Python is pretty good about displaying sensible errors for complex value discrepancies in test assertions.
|
||||
assert json.loads(buffer.to_request())['streams'][0]['values'][0][1] == 'Some test log line'
|
||||
|
||||
|
||||
def test_log_handler_post():
|
||||
'''
|
||||
Assert that the flush function sends a post request after a certain limit
|
||||
'''
|
||||
handler = module.Loki_log_handler(flexmock(), False)
|
||||
flexmock(module.requests).should_receive('post').and_return(
|
||||
flexmock(raise_for_status=lambda: '')
|
||||
).once()
|
||||
for num in range(int(module.MAX_BUFFER_LINES * 1.5)):
|
||||
handler.raw(num)
|
||||
|
||||
|
||||
def test_log_handler_post_failiure():
|
||||
witten marked this conversation as resolved
Outdated
witten
commented
Nice use of mocking. Although personally I'm not above calling Nice use of mocking. Although personally I'm not above calling `logging.makeLogRecord()` in tests...
|
||||
'''
|
||||
Assert that the flush function catches request exceptions
|
||||
'''
|
||||
handler = module.Loki_log_handler(flexmock(), False)
|
||||
flexmock(module.requests).should_receive('post').and_return(
|
||||
flexmock(raise_for_status=lambda: (_ for _ in ()).throw(requests.RequestException()))
|
||||
witten marked this conversation as resolved
witten
commented
Why not just do an assert that the Why not just do an assert that the `to_request()` value is the expected encoded JSON string? Why bother decoding it? Presumably any JSON string you put into a test is known-valid.
IBims1NicerTobi
commented
Mainly whitespace etc. It's more consistent this way and we don't have to worry about anything formatting related. In the end I don't care that the strings are the same but that the json is the same. Mainly whitespace etc. It's more consistent this way and we don't have to worry about anything formatting related. In the end I don't care that the strings are the same but that the json is the same.
|
||||
).once()
|
||||
for num in range(int(module.MAX_BUFFER_LINES * 1.5)):
|
||||
handler.raw(num)
|
||||
|
||||
|
||||
def test_log_handler_empty_flush_noop():
|
||||
'''
|
||||
Test that flushing an empty buffer does indeed nothing
|
||||
'''
|
||||
handler = module.Loki_log_handler(flexmock(), False)
|
||||
handler.flush()
|
Cool! I didn't know you could do this.