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 validations for duplicate JMX bean entries #11505

Merged
merged 9 commits into from
Jul 14, 2022

Conversation

steveny91
Copy link
Contributor

@steveny91 steveny91 commented Feb 15, 2022

What does this PR do?

Add logic to check if any duplicate bean + attribute combination exist in a metrics.yaml

Motivation

To prevent the same data to be collected 2 or more times

Additional Notes

Tested it against the confluence_platform's and hive's metric.yaml and was able to find a couple of duplicate beans. Examples:
Link1 versus Link2

Sample output:

Validating JMX metrics files for 13 checks ...
confluent_platform:
    - The following bean and attribute combination is a duplicate:
    - kafka.controller:type=KafkaController,name=OfflinePartitionsCount: ['Value']
hive:
    - The following bean and attribute combination is a duplicate:
    - metrics:name=memory.heap.init: ['Value']
    - metrics:name=memory.heap.usage: ['Value']
    - metrics:name=memory.heap.used: ['Value']
    - metrics:name=memory.heap.max: ['Value']
    - metrics:name=memory.heap.committed: ['Value']
    - metrics:name=memory.non-heap.init: ['Value']
    - metrics:name=memory.non-heap.used: ['Value']
    - metrics:name=memory.non-heap.max: ['Value']
    - metrics:name=memory.non-heap.committed: ['Value']
    - metrics:name=memory.total.init: ['Value']
    - metrics:name=memory.total.usage: ['Value']
    - metrics:name=memory.total.used: ['Value']
    - metrics:name=memory.total.max: ['Value']
    - metrics:name=memory.total.committed: ['Value']
13 total JMX integrations
11 valid metrics files
2 invalid metrics files

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 Feb 15, 2022

Codecov Report

Merging #11505 (18e3af3) into master (503da2c) will increase coverage by 0.01%.
The diff coverage is n/a.

Flag Coverage Δ
ambari 85.75% <ø> (ø)
arangodb 98.21% <ø> (ø)
avi_vantage 91.92% <ø> (ø)
boundary 100.00% <ø> (ø)
btrfs 82.91% <ø> (ø)
cacti 83.95% <ø> (ø)
calico 83.33% <ø> (ø)
cilium 75.34% <ø> (+0.93%) ⬆️
citrix_hypervisor 87.50% <ø> (ø)
cloud_foundry_api 95.98% <ø> (+0.12%) ⬆️
crio 89.79% <ø> (ø)
datadog_cluster_agent 90.00% <ø> (ø)
dns_check 93.84% <ø> (ø)
eks_fargate 94.05% <ø> (ø)
istio 77.65% <ø> (+0.55%) ⬆️
kube_controller_manager 96.85% <ø> (ø)
kube_dns 98.85% <ø> (ø)
mapr 82.70% <ø> (ø)
network 78.78% <ø> (+0.88%) ⬆️
openldap 96.33% <ø> (ø)
postfix 88.04% <ø> (ø)
prometheus 94.17% <ø> (ø)

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

@steveny91 steveny91 marked this pull request as ready for review February 15, 2022 21:24
@steveny91 steveny91 requested a review from a team as a code owner February 15, 2022 21:24
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 good! I tried it out and it caught some errors so can you create a PR to fix them too? Also, it didn't detect duplicate keys, which were present in this file at this commit: https://github.com/DataDog/integrations-core/blob/9975181896f845d3da3dd2595bc01e8daf9d552c/confluent_platform/datadog_checks/confluent_platform/data/metrics.yaml. There is some discussion about how to do that here: https://gist.github.com/pypt/94d747fe5180851196eb since pyyaml does not catch them when loading: yaml/pyyaml#165

@steveny91
Copy link
Contributor Author

steveny91 commented Feb 16, 2022

Looks good! I tried it out and it caught some errors so can you create a PR to fix them too? Also, it didn't detect duplicate keys, which were present in this file at this commit: https://github.com/DataDog/integrations-core/blob/9975181896f845d3da3dd2595bc01e8daf9d552c/confluent_platform/datadog_checks/confluent_platform/data/metrics.yaml. There is some discussion about how to do that here: https://gist.github.com/pypt/94d747fe5180851196eb since pyyaml does not catch them when loading: yaml/pyyaml#165

@sarah-witt By key I'm guessing you're referring to the beans? If so, I had ask Ofek about it prior about the following:

bean:'my.bean:type=foo,name=baz'
  attribute:
    - Count
bean:'my.bean:type=foo,name=baz'
  attribute:
    - Rate

He told me that these shouldn't be flagged


I'd be happy to update the duplicates in our repo!

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.

Thanks! And ah sorry, that's true we shouldn't check for duplicate beans. What I was trying to say was we want to catch fully duplicate entries ie

bean:'my.bean:type=foo,name=baz'
  attribute:
    - Count
bean:'my.bean:type=foo,name=baz'
  attribute:
    - Count

@steveny91
Copy link
Contributor Author

Thanks! And ah sorry, that's true we shouldn't check for duplicate beans. What I was trying to say was we want to catch fully duplicate entries ie

bean:'my.bean:type=foo,name=baz'
  attribute:
    - Count
bean:'my.bean:type=foo,name=baz'
  attribute:
    - Count

@sarah-witt I added some suggestion from the links you provided, thanks! I tested it against the commit you referenced and seems to be working there.

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.

Thanks, this looks awesome! I just had a small wording nit

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.

🎉

@steveny91 steveny91 merged commit 4a19300 into master Jul 14, 2022
@steveny91 steveny91 deleted the steveny91/jmx-config-validations branch July 14, 2022 14:15
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.

2 participants