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 option to dynamically determine what APIs to query for metrics #10815

Merged
merged 42 commits into from
Dec 14, 2021

Conversation

sarah-witt
Copy link
Contributor

@sarah-witt sarah-witt commented Dec 8, 2021

What does this PR do?

  • Adds an option to dynamically determine what APIs to query for metrics, so if some nginx modules are not enabled, then their corresponding endpoints will not be queried in the check
  • this option will be enabled by default on new installs

Motivation

Some nginx modules and APIs can be disabled, and currently, when they are, their corresponding endpoints are still queried in the integration, and error logs show up in the integration and in nginx logs

Additional Notes

This was tested on an nginx plus server, where stream is not enabled, and use_plus_api_stream is enabled. Warning logs are not seen

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

Co-authored-by: Jorie Helwig <jorie.helwig@datadoghq.com>
nginx/tests/test_unit.py Outdated Show resolved Hide resolved
nginx/tests/test_unit.py Outdated Show resolved Hide resolved
yzhan289
yzhan289 previously approved these changes Dec 13, 2021
nginx/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
nginx/datadog_checks/nginx/nginx.py Outdated Show resolved Hide resolved
Co-authored-by: Christine Chen <ChristineTChen@users.noreply.github.com>
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@sarah-witt sarah-witt merged commit 3cea8fc into master Dec 14, 2021
@sarah-witt sarah-witt deleted the sarah/nginx-determine-apis branch December 14, 2021 19:55
cswatt pushed a commit that referenced this pull request Jan 5, 2022
…10815)

* Add test data and limit_reqs endpoint

* Add support for new endpoints in check

* Update metadata with new metrics

* add more tests for tags

* Add more tests and http limit conns endpoint

* Add tests for all plus api versions

* Reformat code and fix typos

* Remove unneeded mappings

* Remove unneeded mapping and test fixture

* Properly handle metrics that should be counts

* Change more metrics to count

* Fix short names of metrics

* Update style and add assertion for all count metrics

* Update conf.example with new description

* Update test folder structure and add mapping for plus api endpoints

* Assert metadata in all tests

* Add missing count metrics, assert metrics covered, and refactor tests

* Remove slashes in directories

* Begin _get_enabled_apis to dynamically determine enabled apis

* Add fixtures

* Comple _get_enabled_endpoints and add more fixtures and test

* Add config models and option

* Add more tests for only_query_enabled_endpoints

* Update config models and remove conflicts

* Update endpoint mapping and fix mock return values

* Add comments and more logging

* Add more log lines and assert them

* Don't mention stream disabled when only_query_enabled_endpoints is enabled

* Add another test for edge cases

* Use more correct url for test

* Apply suggestions from code review

Co-authored-by: Jorie Helwig <jorie.helwig@datadoghq.com>

* Fix style

* Make endpoints a set

* Use dd_run_check

* Remove unused caplog

* Update nginx/assets/configuration/spec.yaml

Co-authored-by: Christine Chen <ChristineTChen@users.noreply.github.com>

* Sync example config

* Use urljoin

Co-authored-by: Jorie Helwig <jorie.helwig@datadoghq.com>
Co-authored-by: Christine Chen <ChristineTChen@users.noreply.github.com>
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

5 participants