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

Add support for Index Statistics metrics #10784

Merged
merged 39 commits into from
Dec 15, 2021

Conversation

steveny91
Copy link
Contributor

What does this PR do?

Add support for the collection of metrics from the Couchbase Index Statistics API that was released in version 7 of Couchbase

Motivation

Feature Request

Additional Notes

https://docs.couchbase.com/server/current/rest-api/rest-index-stats.html

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • 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

@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #10784 (48ff4d3) into master (bde55e9) will increase coverage by 0.00%.
The diff coverage is 92.59%.

Flag Coverage Δ
couchbase 83.46% <92.59%> (+2.00%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

Docs review

couchbase/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
couchbase/metadata.csv Outdated Show resolved Hide resolved
couchbase/metadata.csv Outdated Show resolved Hide resolved
couchbase/metadata.csv Outdated Show resolved Hide resolved
couchbase/metadata.csv Outdated Show resolved Hide resolved
couchbase/metadata.csv Outdated Show resolved Hide resolved
couchbase/metadata.csv Outdated Show resolved Hide resolved
couchbase/metadata.csv Outdated Show resolved Hide resolved
couchbase/metadata.csv Outdated Show resolved Hide resolved
couchbase/metadata.csv Outdated Show resolved Hide resolved
ruthnaebeck
ruthnaebeck previously approved these changes Dec 3, 2021
Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

👍 for docs

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

@sarah-witt sarah-witt left a comment

Choose a reason for hiding this comment

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

Awesome!

couchbase/datadog_checks/couchbase/couchbase.py Outdated Show resolved Hide resolved
couchbase/datadog_checks/couchbase/couchbase.py Outdated Show resolved Hide resolved
couchbase/tests/test_couchbase.py Show resolved Hide resolved
couchbase/tests/test_couchbase.py Outdated Show resolved Hide resolved
couchbase/tests/test_unit.py Outdated Show resolved Hide resolved
couchbase/tests/test_couchbase.py Outdated Show resolved Hide resolved
couchbase/metadata.csv Outdated Show resolved Hide resolved
couchbase/metadata.csv Outdated Show resolved Hide resolved
Copy link
Contributor

@sarah-witt sarah-witt left a comment

Choose a reason for hiding this comment

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

Looks great, I just had two comments about the new version logic

couchbase/datadog_checks/couchbase/couchbase.py Outdated Show resolved Hide resolved
couchbase/datadog_checks/couchbase/couchbase.py Outdated Show resolved Hide resolved
sarah-witt
sarah-witt previously approved these changes Dec 8, 2021
Copy link
Contributor

@sarah-witt sarah-witt left a comment

Choose a reason for hiding this comment

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

Awesome!

return formatted_tags

def _submit_index_node_metrics(self, mname, mval, tags):
index_state_map = {'Active': 0, 'Pause': 1, 'Warmup': 2}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is constant, so let's move it out of the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it to couchbase_consts.py and importing it.

# Error handling in case Couchbase changes their versioning format
if self._version and int(self._version.split(".")[0]) < 7:
self.log.warning(
"Index Stats Metrics are only available in Couchbase version 7+. Detected version: %",
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this option is behind a flag that must be enabled by the user and we properly documented the requirement, I don't know if we need this warning log. We could consider just validating the version in check() before we call _collect_index_stats_metrics, and if version is incompatible, avoid collecting completely versus running into a requests error and log a debug line.

Elastic and postgres have examples of checking versions before collecting a subset of metrics

Copy link
Contributor Author

@steveny91 steveny91 Dec 14, 2021

Choose a reason for hiding this comment

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

I think I initially approached it in a way that we should try to get the index stats anyways in case the version logic would fail or if for whatever reason Couchbase decides to change the way they expose their version.

I moved the logic to check for index_stats_url and version and version >= 7 into check(). I left the try should should the int(self._version.split(".")[0]) not return an integer for whatever reason.

]
return formatted_tags

def _submit_index_node_metrics(self, mname, mval, tags):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def _submit_index_node_metrics(self, mname, mval, tags):
def _submit_index_node_metrics(self, mname, mval):

this function is only used once and passes self.tags which can be called directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. I added your change and attach tags via self._tags in the submit function.

ChristineTChen
ChristineTChen previously approved these changes Dec 14, 2021
Copy link
Contributor

@yzhan289 yzhan289 left a comment

Choose a reason for hiding this comment

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

lgtm just one small nit

data = self._get_stats(url)
except requests.exceptions.RequestException as e:
msg = "Error accessing the Index Statistics endpoint: %s: %s" % (url, str(e))
self.log.warning(str(e))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.log.warning(str(e))
self.log.warning(msg)

@sarah-witt sarah-witt merged commit 202ce7a into master Dec 15, 2021
@sarah-witt sarah-witt deleted the steveny91/couchbase_index_stats branch December 15, 2021 19:57
cswatt pushed a commit that referenced this pull request Jan 5, 2022
* added couchbase 7 with index stats endpoint as a test environment

* Initial metric parsing for index stats

* updated config model and spec to include the new parameter needed to collect index metrics

* styling fix

* more descriptive service check message

* integration test for index stats metric collection

* slight logic refactor

* added units test and refactor metric parsing logic

* smaller sample bucket for testing

* add new metrics to metadata.csv

* add comments for documentations and touch ups on tests

* fix typo

* lower timeout on tests

* attempt at fixing flaky test

* fix metric descriptions and config param description

* style fix

* removal of log line and minor description change

* incorporated changes from reviews

* switch unit test to use parameterize version

* change tests and small refactor to tag parsing logic

* change few metrics to count metrics

* styling

* add one unit test for tag extraction

* styling

* add missing unit for metric

* small nits and metadata edits

* styling

* small tweak in test

* warn log for index stats requirement not met

* typo in comments

* grammaticaal fix and error handling for version

* add more error handling

* remove unused import

* logic error fix

* Better version check logic in test

Co-authored-by: Andrew Zhang <[email protected]>

* small refactor

* refactor

* slight change to log message

Co-authored-by: Andrew Zhang <[email protected]>
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.

5 participants