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

Collect metrics if a client token is available #5177

Merged
merged 3 commits into from
Dec 18, 2019
Merged

Collect metrics if a client token is available #5177

merged 3 commits into from
Dec 18, 2019

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Dec 7, 2019

Additional Notes

Also, ensures consistent tags. Previously:

  • Metric tags would depend on the method execution order
  • Service checks were tagged by dynamic values, leading to alerts that could never resolve in the backend

@codecov
Copy link

codecov bot commented Dec 9, 2019

Codecov Report

Merging #5177 into master will increase coverage by 5.31%.
The diff coverage is 92.88%.

Impacted Files Coverage Δ
vault/tests/common.py 88.88% <100%> (+3.17%) ⬆️
vault/datadog_checks/vault/metrics.py 100% <100%> (ø)
vault/tests/metrics.py 100% <100%> (ø)
vault/tests/utils.py 75% <75%> (ø)
vault/tests/test_integration.py 87.5% <86.04%> (ø) ⬆️
vault/tests/conftest.py 83.11% <93.33%> (+3.52%) ⬆️
vault/datadog_checks/vault/vault.py 91.82% <95%> (+2.74%) ⬆️
vault/tests/test_vault.py 97.14% <96.15%> (+0.33%) ⬆️
...checks_dev/tests/plugin/test_environment_runner.py
oracle/tests/test_oracle.py
... and 833 more

@ofek ofek force-pushed the ofek/v branch 2 times, most recently from 7398634 to 9619bd8 Compare December 9, 2019 05:07
l0k0ms
l0k0ms previously approved these changes Dec 9, 2019
Copy link
Contributor

@l0k0ms l0k0ms left a comment

Choose a reason for hiding this comment

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

G2G doc wise :)

Copy link
Contributor

@FlorianVeaux FlorianVeaux left a comment

Choose a reason for hiding this comment

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

Reviewed just a part for now, still missing metadata and there are still a few things I don't understand regarding how you setup tests.

vault/datadog_checks/vault/vault.py Show resolved Hide resolved
tags = list(self._tags)
tags.extend(dynamic_tags)
for submit_function in submission_queue:
submit_function(tags=tags)
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 a more abstract logic but I don't think we can do better without adding more api calls. In any case that's 10 times better than the previous logic

vault/datadog_checks/vault/vault.py Outdated Show resolved Hide resolved
vault/datadog_checks/vault/vault.py Show resolved Hide resolved
vault/datadog_checks/vault/vault.py Show resolved Hide resolved
self.log.exception(msg)
raise ApiUnreachable
self.service_check(self.SERVICE_CHECK_CONNECT, self.CRITICAL, message=msg, tags=self._tags)
raise ApiUnreachable(msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the Vault API fails here, we won't collect prometheus metrics. I'd say it's worth to catch the error and collect prometheus metrics as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would would the service check be if only a subset of calls succeed?

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 fair, or else we can have one service check for the API and one for prometheus?

vault/datadog_checks/vault/vault.py Outdated Show resolved Hide resolved
vault/datadog_checks/vault/vault.py Show resolved Hide resolved
Copy link
Contributor

@FlorianVeaux FlorianVeaux left a comment

Choose a reason for hiding this comment

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

ofek and others added 3 commits December 17, 2019 02:18
* Add audit, core, token, runtime  metrics

* add auth, WAL metric

* Remove duplicate

* add replication metrics pt1

* Add replication metrics and secrets engine metrics

* Finish adding storage backend metrics

* Fix metric type

* Fix policy summary description

* Fix typos

* Keep capitalized secrets engines metrics
@ofek
Copy link
Contributor Author

ofek commented Dec 17, 2019

Fixed by #5224

@ofek ofek merged commit d6bd614 into master Dec 18, 2019
@ofek ofek deleted the ofek/v branch December 18, 2019 15:32
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.

None yet

4 participants