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 124 additions and 2 deletions
Showing only changes of commit 9e2674ea5a - Show all commits

View File

@ -132,8 +132,6 @@ def ping_monitor(hook_config, config, config_filename, state, monitoring_log_lev
'''
Add an entry to the loki logger with the current state.
'''
if dry_run:
return
for handler in tuple(logging.getLogger().handlers):
if isinstance(handler, Loki_log_handler):
if state in MONITOR_STATE_TO_LOKI.keys():

View File

@ -0,0 +1,124 @@
from flexmock import flexmock
from borgmatic.hooks import loki
import json
import platform
import logging
import requests
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_added():
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!
hook_config = {'url': 'http://localhost:3100/loki/api/v1/push', 'labels': {'app': 'borgmatic'}}
config_filename = 'test.yaml'
dry_run = True
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.
loki.initialize_monitor(hook_config, '', config_filename, '', dry_run)
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.
for handler in tuple(logging.getLogger().handlers):
if isinstance(handler, loki.Loki_log_handler):
assert True
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.
return
assert False
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_ping():
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.
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
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.
return
assert False
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_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
witten marked this conversation as resolved Outdated

You could use the any() idiom here too.

You could use the `any()` idiom here too.
def test_log_handler_gets_labels():
buffer = loki.Loki_log_buffer('', False)
buffer.add_label('test', 'label')
assert buffer.root['streams'][0]['stream']['test'] == 'label'
buffer.add_label('test2', 'label2')
assert buffer.root['streams'][0]['stream']['test2'] == 'label2'
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', } ```
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
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.
return
assert False
def test_log_handler_gets_logs():
buffer = loki.Loki_log_buffer('', 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
def test_log_handler_gets_raw():
handler = loki.Loki_log_handler('', False)
handler.emit(flexmock(getMessage=lambda: 'Some test log line'))
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 len(handler.buffer) == 1
def test_log_handler_json():
buffer = loki.Loki_log_buffer('', False)
assert json.loads(buffer.to_request()) == json.loads('{"streams":[{"stream":{},"values":[]}]}')
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.
def test_log_handler_json_labels():
buffer = loki.Loki_log_buffer('', 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)
buffer.add_value('Some test log line')
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(
flexmock(raise_for_status=lambda: '')
).once()
witten marked this conversation as resolved Outdated

Good. I'm glad to see this code path has some coverage!

Good. I'm glad to see this code path has some coverage!
for x in range(150):
witten marked this conversation as resolved Outdated

Instead of 150, you could do loki.MAX_BUFFER_LINES + 1 or even loki.MAX_BUFFER_LINES * 1.5.

Do not feel strongly. Magic numbers are okay-er in tests than in actual code IMO.

Instead of `150`, you could do `loki.MAX_BUFFER_LINES + 1` or even `loki.MAX_BUFFER_LINES * 1.5`. Do not feel strongly. Magic numbers are okay-er in tests than in actual code IMO.
handler.raw(x)
def test_post_failiure():
handler = loki.Loki_log_handler('', False)
flexmock(loki.requests).should_receive('post').and_return(
flexmock(raise_for_status=lambda: (_ for _ in ()).throw(requests.RequestException()))
).once()
for x in range(150):
handler.raw(x)
def test_empty_flush():
witten marked this conversation as resolved Outdated

Since there are no assertions here, I'd encode that fact into the test name. Example:test_loki_log_handler_flush_does_not_raise(). That says: We're just running through the motions here to shake out any stupid bugs but not asserting anything.

Since there are no assertions here, I'd encode that fact into the test name. Example:`test_loki_log_handler_flush_does_not_raise()`. That says: We're just running through the motions here to shake out any stupid bugs but not asserting anything.
handler = loki.Loki_log_handler('', False)
handler.flush()