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.

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

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

Merged
merged 16 commits into from
Oct 23, 2019

Conversation

AlexandreYang
Copy link
Member

@AlexandreYang AlexandreYang commented Oct 16, 2019

What does this PR do?

  1. Add more tests for agent signature

  2. Fix made to handle this case where instances is not passed when using agent 5 signature

AgentCheck('check_name', {'init_conf1': 'init_value1'}, {'agent_conf1': 'agent_value1'})
OpenMetricsBaseCheck('openmetrics_check', self.INIT_CONFIG, self.AGENT_CONFIG)

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

@codecov
Copy link

codecov bot commented Oct 16, 2019

Codecov Report

Merging #4784 into master will decrease coverage by 0.22%.
The diff coverage is 100%.

Impacted Files Coverage Δ
...g_checks_base/tests/test_openmetrics_base_check.py 100% <100%> (ø) ⬆️
...dog_checks_base/datadog_checks/base/checks/base.py 79.28% <100%> (ø) ⬆️
...tadog_checks/base/checks/openmetrics/base_check.py 87.23% <100%> (+2.78%) ⬆️
datadog_checks_base/tests/test_agent_check.py 99.39% <100%> (ø) ⬆️
vault/tests/common.py 85.71% <0%> (-14.29%) ⬇️
rabbitmq/tests/conftest.py 96.49% <0%> (-1.76%) ⬇️
vault/tests/test_vault.py 96.8% <0%> (-0.06%) ⬇️
windows_service/tests/common.py
oracle/tests/test_oracle.py
gearmand/datadog_checks/gearmand/gearmand.py
... and 172 more

@AlexandreYang AlexandreYang changed the title Add more tests for agent signature Add more tests for agent signature and fix no instances case Oct 16, 2019
@AlexandreYang AlexandreYang marked this pull request as ready for review October 16, 2019 11:49
@AlexandreYang AlexandreYang requested review from a team as code owners October 16, 2019 11:49
@AlexandreYang AlexandreYang changed the title Add more tests for agent signature and fix no instances case Fix no instances case for AgentCheck signature and add more tests Oct 16, 2019
@AlexandreYang AlexandreYang changed the title Fix no instances case for AgentCheck signature and add more tests [HOLD] Fix no instances case for AgentCheck signature and add more tests Oct 16, 2019
@AlexandreYang
Copy link
Member Author

In production, the AgentCheck is run by the datadog-agent, so, I guess the case should not be a real problem since the datadog-agent is probably always passing valid parameters.

Hence, setting this PR on HOLD to be merged later.

@AlexandreYang AlexandreYang changed the title [HOLD] Fix no instances case for AgentCheck signature and add more tests [HOLD, merge after 6.15 release] Fix no instances case for AgentCheck signature and add more tests Oct 16, 2019

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

@AlexandreYang AlexandreYang changed the title [HOLD, merge after 6.15 release] Fix no instances case for AgentCheck signature and add more tests Fix no instances case for AgentCheck signature and add more tests Oct 22, 2019
@AlexandreYang AlexandreYang dismissed hithwen’s stale review October 23, 2019 18:08

Changes requested have been addressed

@AlexandreYang AlexandreYang merged commit 57e8ead into master Oct 23, 2019
@AlexandreYang AlexandreYang deleted the alex/agent_signature_qa branch October 23, 2019 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants