Added support for grafana loki #747

Merged
witten merged 5 commits from :main into main 2023-08-25 16:28:20 +00:00
2 changed files with 134 additions and 78 deletions
Showing only changes of commit 099a712e53 - Show all commits

View File

@ -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

View File

@ -1,74 +1,27 @@
from flexmock import flexmock
from borgmatic.hooks import loki
import json
import platform
import logging
import requests
from flexmock import flexmock
def test_log_handler_gets_added():
hook_config = {'url': 'http://localhost:3100/loki/api/v1/push', 'labels': {'app': 'borgmatic'}}
config_filename = 'test.yaml'
dry_run = True
loki.initialize_monitor(hook_config, '', config_filename, '', dry_run)
for handler in tuple(logging.getLogger().handlers):
if isinstance(handler, loki.Loki_log_handler):
assert True
return
assert False
def test_ping():
hook_config = {'url': 'http://localhost:3100/loki/api/v1/push', 'labels': {'app': 'borgmatic'}}
config_filename = 'test.yaml'
dry_run = True
loki.initialize_monitor(hook_config, '', config_filename, '', dry_run)
loki.ping_monitor(hook_config, '', config_filename, loki.monitor.State.FINISH, '', dry_run)
for handler in tuple(logging.getLogger().handlers):
if isinstance(handler, loki.Loki_log_handler):
assert len(handler.buffer) <= 1
return
assert False
def test_log_handler_gets_removed():
hook_config = {'url': 'http://localhost:3100/loki/api/v1/push', 'labels': {'app': 'borgmatic'}}
config_filename = 'test.yaml'
dry_run = True
loki.initialize_monitor(hook_config, '', config_filename, '', dry_run)
loki.destroy_monitor(hook_config, '', config_filename, '', dry_run)
for handler in tuple(logging.getLogger().handlers):
if isinstance(handler, loki.Loki_log_handler):
assert False
from borgmatic.hooks import loki as module
witten marked this conversation as resolved
Review

There's a convention in this codebase to import the module under test as module. E.g.:

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.

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.
Review

Thats fixed now

Thats fixed now
def test_log_handler_gets_labels():
witten marked this conversation as resolved
Review

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.

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.
Review

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!
buffer = loki.Loki_log_buffer('', False)
'''
Assert that adding labels works
'''
witten marked this conversation as resolved Outdated

You could move these inline to the function call as keyword arguments (... , config_filename='test.yaml', ...) since they're only used once.

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

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.

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
Review

You could safely delete this assert since it'll always be true.

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
Review

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.

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
Review

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.

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_handler_label_replacment():
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
loki.initialize_monitor(hook_config, '', config_filename, '', dry_run)
for handler in tuple(logging.getLogger().handlers):
if isinstance(handler, loki.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_log_handler_gets_logs():
buffer = loki.Loki_log_buffer('', False)
def test_log_buffer_gets_raw():
'''
witten marked this conversation as resolved Outdated

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.

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
@ -76,49 +29,70 @@ def test_log_handler_gets_logs():
assert len(buffer) == 2
witten marked this conversation as resolved
Review

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?

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`?
Review

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
Review

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.

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_handler_gets_raw():
handler = loki.Loki_log_handler('', False)
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_handler_json():
buffer = loki.Loki_log_buffer('', False)
def test_log_buffer_json():
'''
witten marked this conversation as resolved Outdated

You could use the any() idiom here too.

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_handler_json_labels():
buffer = loki.Loki_log_buffer('', False)
def test_log_buffer_json_labels():
'''
witten marked this conversation as resolved Outdated

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:

assert buffer.root['streams'][0]['stream'] == {
    'test': 'label',
    'test2': 'label2',
}
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_handler_json_log_lines():
buffer = loki.Loki_log_buffer('', False)
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
Review

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.

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():
handler = loki.Loki_log_handler('', False)
flexmock(loki.requests).should_receive('post').and_return(
'''
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 x in range(150):
handler.raw(x)
for num in range(int(module.MAX_BUFFER_LINES * 1.5)):
handler.raw(num)
def test_post_failiure():
handler = loki.Loki_log_handler('', False)
flexmock(loki.requests).should_receive('post').and_return(
def test_log_handler_post_failiure():
witten marked this conversation as resolved Outdated

Nice use of mocking. Although personally I'm not above calling logging.makeLogRecord() in tests...

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
Review

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.

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.
Review

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 x in range(150):
handler.raw(x)
for num in range(int(module.MAX_BUFFER_LINES * 1.5)):
handler.raw(num)
def test_empty_flush():
handler = loki.Loki_log_handler('', False)
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()