Add detailed_index_stats parameter to pull index-level metrics #10766

missingcharacter · 2021-12-01T03:13:21Z

What does this PR do?

Creates a parameter called detailed_index_stats within the elastic integration.
In order to obtain index-level stats when your cluster is hosted externally (i.e., you're not pointing to localhost), you'd set:

cluster_stats: true
detailed_index_stats: true

Motivation

AWS OpenSearch integration does not have these metrics and I'd like to get as many stats as I can out of my cluster

Additional Notes

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

…th cluster_stats if you want to pull index-level metrics

codecov · 2021-12-01T05:22:54Z

Codecov Report

Merging #10766 (580e516) into master (ea588b8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Flag	Coverage Δ
elastic	`90.52% <100.00%> (+1.89%)`	⬆️

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

elastic/assets/configuration/spec.yaml

Co-authored-by: Jorie Helwig <joriephoto@gmail.com>

sarah-witt

Hi @missingcharacter thanks a lot for the PR! I left two initial comments

sarah-witt · 2021-12-03T15:36:27Z

elastic/tests/test_config.py

+    c = from_instance({'url': 'http://example.com', 'detailed_index_stats': True})
+    assert c.detailed_index_stats is True
+
+


Can you also add a test for running the check when this parameter is enabled, and assert the expected tags?

@sarah-witt thanks and I addressed this comment in commit 0ffcd75

sarah-witt · 2021-12-03T15:40:16Z

elastic/datadog_checks/elastic/elastic.py

            if value is not None:
-                value = value.get(key)
+                value = value.get(key.replace('\\', ''))


Can you add some unit tests for process_metric since the behavior is changing?

I added the creation of an index that starts with a dot "." and added a check for this new index to test_index_metrics and the new test I created called test_detailed_index_stats in commit 0ffcd75

jtappa

👍🏻 from docs team

coignetp

Thanks for the PR! Could you add documentation about these metrics in metadata.csv https://github.com/DataDog/integrations-core/blob/master/elastic/metadata.csv

elastic/tests/test_elastic.py

Co-authored-by: Paul <paul.coignet@datadoghq.com>

… pass

missingcharacter · 2021-12-06T15:06:28Z

@coignetp all tests pass now

coignetp

Sorry for the noice and the back and forth, but in the end let's get rid of assert_all_metrics_covered, as it seems we should apply this more broadly and it's out of scope of this PR

elastic/metadata.csv

elastic/tests/test_elastic.py

Co-authored-by: Paul <paul.coignet@datadoghq.com>

missingcharacter · 2021-12-06T19:06:06Z

@coignetp and @sarah-witt all tests pass now

coignetp

Thanks!

* [elasticsearch] Creating parameter detailed_index_stats to be used with cluster_stats if you want to pull index-level metrics * Update elastic/assets/configuration/spec.yaml Co-authored-by: Jorie Helwig <joriephoto@gmail.com> * [elastic] ddev validate config --sync elastic * [elastic] Adding tests for when an index name start with a dot "." * [elastic] Asserting all metrics instead of 1 at a time Co-authored-by: Paul <paul.coignet@datadoghq.com> * [tests] [detailed_index_stats] Asserting rest of metrics for tests to pass * [elastic] Adding correct columns to elastic/metadata.csv * [elastic] Adding missing metrics to metadata.csv * [elastic] Remove system.* metrics from metadata.csv for now Co-authored-by: Paul <paul.coignet@datadoghq.com> * [elastic] Remove system.* metrics from assert_metrics_using_metadata Co-authored-by: Paul <paul.coignet@datadoghq.com> * [elastic] Remove aggregator.assert_all_metrics_covered() Co-authored-by: Paul <paul.coignet@datadoghq.com> * [elastic] python-black on elastic/tests/test_elastic.py Co-authored-by: Jorie Helwig <joriephoto@gmail.com> Co-authored-by: Paul <paul.coignet@datadoghq.com> c6f368f

missingcharacter · 2021-12-07T12:15:02Z

Thanks @coignetp @jtappa @sarah-witt

* [elasticsearch] Creating parameter detailed_index_stats to be used with cluster_stats if you want to pull index-level metrics * Update elastic/assets/configuration/spec.yaml Co-authored-by: Jorie Helwig <joriephoto@gmail.com> * [elastic] ddev validate config --sync elastic * [elastic] Adding tests for when an index name start with a dot "." * [elastic] Asserting all metrics instead of 1 at a time Co-authored-by: Paul <paul.coignet@datadoghq.com> * [tests] [detailed_index_stats] Asserting rest of metrics for tests to pass * [elastic] Adding correct columns to elastic/metadata.csv * [elastic] Adding missing metrics to metadata.csv * [elastic] Remove system.* metrics from metadata.csv for now Co-authored-by: Paul <paul.coignet@datadoghq.com> * [elastic] Remove system.* metrics from assert_metrics_using_metadata Co-authored-by: Paul <paul.coignet@datadoghq.com> * [elastic] Remove aggregator.assert_all_metrics_covered() Co-authored-by: Paul <paul.coignet@datadoghq.com> * [elastic] python-black on elastic/tests/test_elastic.py Co-authored-by: Jorie Helwig <joriephoto@gmail.com> Co-authored-by: Paul <paul.coignet@datadoghq.com>

missingcharacter requested review from a team as code owners December 1, 2021 03:13

[elasticsearch] Creating parameter detailed_index_stats to be used wi…

bd4fcd5

…th cluster_stats if you want to pull index-level metrics

therve added changelog/Added integration/elastic labels Dec 1, 2021

jtappa suggested changes Dec 1, 2021

View reviewed changes

elastic/assets/configuration/spec.yaml Outdated Show resolved Hide resolved

missingcharacter and others added 2 commits December 1, 2021 16:17

Update elastic/assets/configuration/spec.yaml

09e900d

Co-authored-by: Jorie Helwig <joriephoto@gmail.com>

[elastic] ddev validate config --sync elastic

a617583

sarah-witt requested changes Dec 3, 2021

View reviewed changes

jtappa previously approved these changes Dec 3, 2021

View reviewed changes

missingcharacter dismissed jtappa’s stale review via 0a51e74 December 5, 2021 17:07

[elastic] Adding tests for when an index name start with a dot "."

0ffcd75

coignetp requested changes Dec 6, 2021

View reviewed changes

elastic/tests/test_elastic.py Show resolved Hide resolved

coignetp changed the title ~~[Added] Creating parameter detailed_index_stats to be used with cluster_stats if you want to pull index-level metrics~~ Add detailed_index_stats parameter to pull index-level metrics Dec 6, 2021

missingcharacter and others added 4 commits December 6, 2021 05:07

[elastic] Asserting all metrics instead of 1 at a time

e2a5f12

Co-authored-by: Paul <paul.coignet@datadoghq.com>

[tests] [detailed_index_stats] Asserting rest of metrics for tests to…

3b828c9

… pass

[elastic] Adding correct columns to elastic/metadata.csv

0a78387

[elastic] Adding missing metrics to metadata.csv

d95ea66

coignetp requested changes Dec 6, 2021

View reviewed changes

elastic/metadata.csv Outdated Show resolved Hide resolved

elastic/tests/test_elastic.py Outdated Show resolved Hide resolved

elastic/tests/test_elastic.py Outdated Show resolved Hide resolved

missingcharacter and others added 4 commits December 6, 2021 12:40

[elastic] Remove system.* metrics from metadata.csv for now

12c4a9c

Co-authored-by: Paul <paul.coignet@datadoghq.com>

[elastic] Remove system.* metrics from assert_metrics_using_metadata

ba00154

Co-authored-by: Paul <paul.coignet@datadoghq.com>

[elastic] Remove aggregator.assert_all_metrics_covered()

fbfba82

Co-authored-by: Paul <paul.coignet@datadoghq.com>

[elastic] python-black on elastic/tests/test_elastic.py

580e516

coignetp approved these changes Dec 7, 2021

View reviewed changes

coignetp merged commit c6f368f into DataDog:master Dec 7, 2021

missingcharacter deleted the aws-opensearch-index-level-metrics branch December 7, 2021 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add detailed_index_stats parameter to pull index-level metrics #10766

Add detailed_index_stats parameter to pull index-level metrics #10766

missingcharacter commented Dec 1, 2021 •

edited

Loading

codecov bot commented Dec 1, 2021 •

edited

Loading

sarah-witt left a comment

sarah-witt Dec 3, 2021

missingcharacter Dec 5, 2021

sarah-witt Dec 3, 2021

missingcharacter Dec 5, 2021

jtappa left a comment

coignetp left a comment

missingcharacter commented Dec 6, 2021

coignetp left a comment

missingcharacter commented Dec 6, 2021

coignetp left a comment

missingcharacter commented Dec 7, 2021

		c = from_instance({'url': 'http://example.com', 'detailed_index_stats': True})
		assert c.detailed_index_stats is True

Add detailed_index_stats parameter to pull index-level metrics #10766

Add detailed_index_stats parameter to pull index-level metrics #10766

Conversation

missingcharacter commented Dec 1, 2021 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

codecov bot commented Dec 1, 2021 • edited Loading

Codecov Report

sarah-witt left a comment

Choose a reason for hiding this comment

sarah-witt Dec 3, 2021

Choose a reason for hiding this comment

missingcharacter Dec 5, 2021

Choose a reason for hiding this comment

sarah-witt Dec 3, 2021

Choose a reason for hiding this comment

missingcharacter Dec 5, 2021

Choose a reason for hiding this comment

jtappa left a comment

Choose a reason for hiding this comment

coignetp left a comment

Choose a reason for hiding this comment

missingcharacter commented Dec 6, 2021

coignetp left a comment

Choose a reason for hiding this comment

missingcharacter commented Dec 6, 2021

coignetp left a comment

Choose a reason for hiding this comment

missingcharacter commented Dec 7, 2021

missingcharacter commented Dec 1, 2021 •

edited

Loading

codecov bot commented Dec 1, 2021 •

edited

Loading