Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix no instances case for AgentCheck signature and add more tests #4784

Merged
merged 16 commits into from
Oct 23, 2019
Merged

Fix no instances case for AgentCheck signature and add more tests #4784

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
10b912b
Add more tests for agent signature
AlexandreYang Oct 16, 2019
16e2bbe
Fix signature
AlexandreYang Oct 16, 2019
946f135
Improve test case
AlexandreYang Oct 16, 2019
10d9c42
add test for agent signature
AlexandreYang Oct 16, 2019
ad8866e
Fix style
AlexandreYang Oct 16, 2019
ca2f967
Improve test
AlexandreYang Oct 16, 2019
717862f
Empty commit to trigger CI
AlexandreYang Oct 16, 2019
42baacc
fix base.py
AlexandreYang Oct 16, 2019
e514e44
fix base.py
AlexandreYang Oct 16, 2019
cee5ab1
add tuple to signature logic
AlexandreYang Oct 16, 2019
49b4c7e
Improve signature doc
AlexandreYang Oct 16, 2019
1a7ab29
Update doc
AlexandreYang Oct 16, 2019
df7bff2
test for tuple and list in OpenMetricsBaseCheck
AlexandreYang Oct 16, 2019
d433980
add tests for tuple
AlexandreYang Oct 16, 2019
fb28f84
Update datadog_checks_base/datadog_checks/base/checks/openmetrics/bas…
AlexandreYang Oct 22, 2019
1d66eb2
Update datadog_checks_base/datadog_checks/base/checks/openmetrics/bas…
AlexandreYang Oct 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion datadog_checks_base/datadog_checks/base/checks/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,21 @@ class except the :py:meth:`check` method but sometimes it might be useful for a
the Agent might create several different Check instances and the method would be
called as many times.

Agent 5 signature:

AgentCheck(name, init_config, agentConfig, instances=None)
AgentCheck.check(instance)

Agent 6,7 signature:

AgentCheck(name, init_config, instances) # instances contain only 1 instance
AgentCheck.check(instance)

Agent 8 signature:

AgentCheck(name, init_config, instance) # one instance
AgentCheck.check() # no more instance argument for check method

:warning: when loading a Custom check, the Agent will inspect the module searching
for a subclass of `AgentCheck`. If such a class exists but has been derived in
turn, it'll be ignored - **you should never derive from an existing Check**.
Expand All @@ -134,7 +149,8 @@ class except the :py:meth:`check` method but sometimes it might be useful for a
if len(args) > 1:
self.init_config = args[1]
if len(args) > 2:
if len(args) > 3 or 'instances' in kwargs:
# agent pass instances as tuple but in test we are usually using list, so we are testing for both
if len(args) > 3 or not isinstance(args[2], (list, tuple)) or 'instances' in kwargs:
# old-style init: the 3rd argument is `agentConfig`
self.agentConfig = args[2]
if len(args) > 3:
Expand Down
18 changes: 17 additions & 1 deletion datadog_checks_base/datadog_checks/base/checks/openmetrics/base_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,17 @@ class OpenMetricsBaseCheck(OpenMetricsScraperMixin, AgentCheck):
metrics:
- bar
- foo


Agent 5 signature:

OpenMetricsBaseCheck(name, init_config, agentConfig, instances=None, default_instances=None,
default_namespace=None)

Agent 6 signature:

OpenMetricsBaseCheck(name, init_config, instances, default_instances=None, default_namespace=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to change to single instance in agent 8?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, from my discussion with @ofek

But maybe should not mention the signature about Agent 8 in this PR, it's a bit out of scope. What do you think ? We can add it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it's mentioned here: https://github.com/DataDog/integrations-core/pull/4784/files#diff-874d5159f166811f6c78ff097251312cR123. Its ok to leave for later but I don't see a reason not to mention it if we know how its going to be

Copy link
Member Author

Choose a reason for hiding this comment

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

@ofek Is that certain that the signature for Agent 8 will like this ?

        Agent 8 signature:
            AgentCheck(name, init_config, instance)     # one instance
            AgentCheck.check()                          # no more instance argument for check method

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the goal


"""

DEFAULT_METRIC_LIMIT = 2000
Expand All @@ -42,12 +53,17 @@ def __init__(self, *args, **kwargs):
self.default_namespace = default_namespace

# pre-generate the scraper configurations

if 'instances' in kwargs:
instances = kwargs['instances']
elif len(args) == 4:
# instances from legacy signature
AlexandreYang marked this conversation as resolved.
Show resolved Hide resolved
instances = args[3]
else:
elif isinstance(args[2], (tuple, list)):
# instances from new signature
AlexandreYang marked this conversation as resolved.
Show resolved Hide resolved
instances = args[2]
else:
instances = None
AlexandreYang marked this conversation as resolved.
Show resolved Hide resolved

if instances is not None:
for instance in instances:
Expand Down
173 changes: 173 additions & 0 deletions datadog_checks_base/tests/test_agent_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,179 @@ def test_warning_args_errors():
assert ["should not raise error: %s"] == check.warnings


@pytest.mark.parametrize(
'case_name, check, expected_attributes',
[
(
'agent 5 signature: only args',
AgentCheck('check_name', {'init_conf1': 'init_value1'}, {'agent_conf1': 'agent_value1'}, [{'foo': 'bar'}]),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {'agent_conf1': 'agent_value1'},
'instance': {'foo': 'bar'},
},
),
(
'agent 5 signature: instances as kwarg',
AgentCheck(
'check_name', {'init_conf1': 'init_value1'}, {'agent_conf1': 'agent_value1'}, instances=[{'foo': 'bar'}]
),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {'agent_conf1': 'agent_value1'},
'instance': {'foo': 'bar'},
},
),
(
'agent 5 signature: agentConfig and instances as kwarg',
AgentCheck(
'check_name',
{'init_conf1': 'init_value1'},
agentConfig={'agent_conf1': 'agent_value1'},
instances=[{'foo': 'bar'}],
),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {'agent_conf1': 'agent_value1'},
'instance': {'foo': 'bar'},
},
),
(
'agent 5 signature: init_config, agentConfig and instances as kwarg',
AgentCheck(
'check_name',
init_config={'init_conf1': 'init_value1'},
agentConfig={'agent_conf1': 'agent_value1'},
instances=[{'foo': 'bar'}],
),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {'agent_conf1': 'agent_value1'},
'instance': {'foo': 'bar'},
},
),
(
'agent 5 signature: name, init_config, agentConfig and instances as kwarg',
AgentCheck(
name='check_name',
init_config={'init_conf1': 'init_value1'},
agentConfig={'agent_conf1': 'agent_value1'},
instances=[{'foo': 'bar'}],
),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {'agent_conf1': 'agent_value1'},
'instance': {'foo': 'bar'},
},
),
(
'agent 5 signature: no instances',
AgentCheck('check_name', {'init_conf1': 'init_value1'}, {'agent_conf1': 'agent_value1'}),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {'agent_conf1': 'agent_value1'},
'instance': None,
},
),
(
'agent 5 signature: no instances and agentConfig as kwarg',
AgentCheck('check_name', {'init_conf1': 'init_value1'}, agentConfig={'agent_conf1': 'agent_value1'}),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {'agent_conf1': 'agent_value1'},
'instance': None,
},
),
(
'agent 5 signature: no instances and init_config, agentConfig as kwarg',
AgentCheck(
'check_name', init_config={'init_conf1': 'init_value1'}, agentConfig={'agent_conf1': 'agent_value1'}
),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {'agent_conf1': 'agent_value1'},
'instance': None,
},
),
(
'agent 5 signature: no instances and name, init_config, agentConfig as kwarg',
AgentCheck(
name='check_name',
init_config={'init_conf1': 'init_value1'},
agentConfig={'agent_conf1': 'agent_value1'},
),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {'agent_conf1': 'agent_value1'},
'instance': None,
},
),
(
'agent 6 signature: only args (instances as list)',
AgentCheck('check_name', {'init_conf1': 'init_value1'}, [{'foo': 'bar'}]),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {},
'instance': {'foo': 'bar'},
},
),
(
'agent 6 signature: only args (instances as tuple)',
AgentCheck('check_name', {'init_conf1': 'init_value1'}, ({'foo': 'bar'},)),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {},
'instance': {'foo': 'bar'},
},
),
(
'agent 6 signature: instances as kwarg',
AgentCheck('check_name', {'init_conf1': 'init_value1'}, instances=[{'foo': 'bar'}]),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {},
'instance': {'foo': 'bar'},
},
),
(
'agent 6 signature: init_config, instances as kwarg',
AgentCheck('check_name', init_config={'init_conf1': 'init_value1'}, instances=[{'foo': 'bar'}]),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {},
'instance': {'foo': 'bar'},
},
),
(
'agent 6 signature: name, init_config, instances as kwarg',
AgentCheck(name='check_name', init_config={'init_conf1': 'init_value1'}, instances=[{'foo': 'bar'}]),
{
'name': 'check_name',
'init_config': {'init_conf1': 'init_value1'},
'agentConfig': {},
'instance': {'foo': 'bar'},
},
),
],
)
def test_agent_signature(case_name, check, expected_attributes):
actual_attributes = {attr: getattr(check, attr) for attr in expected_attributes}
assert expected_attributes == actual_attributes


class TestMetricNormalization:
def test_default(self):
check = AgentCheck()
Expand Down
66 changes: 58 additions & 8 deletions datadog_checks_base/tests/test_openmetrics_base_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,47 +9,97 @@


class TestSignature:
INIT_CONFIG = {'my_init_config': 'foo bar init config'}
AGENT_CONFIG = {'my_agent_config': 'foo bar agent config'}

def test_default_legacy_basic(self):
check = OpenMetricsBaseCheck('openmetrics_check', {}, {})
check = OpenMetricsBaseCheck('openmetrics_check', self.INIT_CONFIG, self.AGENT_CONFIG)

assert check.default_instances == {}
assert check.default_namespace is None
assert check.agentConfig == {'my_agent_config': 'foo bar agent config'}
assert check.name == 'openmetrics_check'
assert check.init_config == {'my_init_config': 'foo bar init config'}

def test_default_instances(self):
def test_default_instances_with_list_instances(self):
instance = {'prometheus_url': 'endpoint'}
check = OpenMetricsBaseCheck('openmetrics_check', {}, [instance], default_namespace='openmetrics')
check = OpenMetricsBaseCheck('openmetrics_check', self.INIT_CONFIG, [instance], default_namespace='openmetrics')

assert check.default_instances == {}
assert check.default_namespace == 'openmetrics'
assert 'endpoint' in check.config_map
assert check.instance == instance
assert check.agentConfig == {}
assert check.name == 'openmetrics_check'
assert check.init_config == {'my_init_config': 'foo bar init config'}

def test_default_instances_with_tuple_instances(self):
instance = {'prometheus_url': 'endpoint'}
check = OpenMetricsBaseCheck(
'openmetrics_check', self.INIT_CONFIG, (instance,), default_namespace='openmetrics'
)

assert check.default_instances == {}
assert check.default_namespace == 'openmetrics'
assert 'endpoint' in check.config_map
assert check.instance == instance
assert check.agentConfig == {}
assert check.name == 'openmetrics_check'
assert check.init_config == {'my_init_config': 'foo bar init config'}

def test_default_instances_legacy(self):
instance = {'prometheus_url': 'endpoint'}
check = OpenMetricsBaseCheck('openmetrics_check', {}, {}, [instance], default_namespace='openmetrics')
check = OpenMetricsBaseCheck(
'openmetrics_check', self.INIT_CONFIG, self.AGENT_CONFIG, [instance], default_namespace='openmetrics'
)

assert check.default_instances == {}
assert check.default_namespace == 'openmetrics'
assert 'endpoint' in check.config_map
assert check.instance == instance
assert check.agentConfig == {'my_agent_config': 'foo bar agent config'}
assert check.name == 'openmetrics_check'
assert check.init_config == {'my_init_config': 'foo bar init config'}

def test_default_instances_legacy_kwarg(self):
instance1 = {'prometheus_url': 'endpoint1'}
instance2 = {'prometheus_url': 'endpoint2'}
check = OpenMetricsBaseCheck(
'openmetrics_check', {}, {}, instances=[instance1, instance2], default_namespace='openmetrics'
'openmetrics_check',
self.INIT_CONFIG,
self.AGENT_CONFIG,
instances=[instance1, instance2],
default_instances={'my_inst': {'foo': 'bar'}},
default_namespace='openmetrics',
)

assert check.default_instances == {}
assert check.default_instances == {'my_inst': {'foo': 'bar'}}
assert check.default_namespace == 'openmetrics'
assert 'endpoint1' in check.config_map
assert 'endpoint2' in check.config_map
assert check.instance == instance1
assert check.agentConfig == {'my_agent_config': 'foo bar agent config'}
assert check.name == 'openmetrics_check'
assert check.init_config == {'my_init_config': 'foo bar init config'}

def test_args_legacy(self):
instance = {'prometheus_url': 'endpoint'}
check = OpenMetricsBaseCheck('openmetrics_check', {}, {}, [instance], None, 'openmetrics')
check = OpenMetricsBaseCheck(
'openmetrics_check',
self.INIT_CONFIG,
self.AGENT_CONFIG,
[instance],
{'my_inst': {'foo': 'bar'}},
'openmetrics',
)

assert check.default_instances == {}
assert check.default_instances == {'my_inst': {'foo': 'bar'}}
assert check.default_namespace == 'openmetrics'
assert 'endpoint' in check.config_map
assert check.instance == instance
assert check.agentConfig == {'my_agent_config': 'foo bar agent config'}
assert check.name == 'openmetrics_check'
assert check.init_config == {'my_init_config': 'foo bar init config'}


def test_rate_override():
Expand Down