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

[receiver/elasticsearch] Add additional metrics #12176

Merged

Conversation

davidji99
Copy link
Contributor

@davidji99 davidji99 commented Jul 8, 2022

Description:
Adding a few more metrics to the elasticsearch receiver.

Please let me know what I missed. This is my first contribution. Thank you.

Documentation:
The documentation was auto-added when running go generate ./...

TODO:

  • Add tests

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 8, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: davidji99 / name: DJ (e7faf1cb6bc158393c78a42b35807a545ec71e8f, a76d481dde4662b381a401c2fd57592fb47d561a, 7797176e15f1de293ad2dbd218adce7a6fc4c6a4, 4eb15783dd69b3ea645d3845a5b484da0f9c7b9b, 90d8e651ced1b36ef676d1aa1c59a0df7c73c1b3)

@davidji99 davidji99 force-pushed the add-more-elasticsearch-metrics branch from 90d8e65 to 831c951 Compare July 8, 2022 10:20
@davidji99 davidji99 changed the title Add more elasticsearch metrics [receiver/elasticsearch] Add additional metrics Jul 8, 2022
@davidji99
Copy link
Contributor Author

@jkowall Any particular reason for thumbs downing this draft PR?

@davidji99 davidji99 marked this pull request as ready for review July 8, 2022 10:40
@davidji99 davidji99 requested a review from a team July 8, 2022 10:40
@jkowall
Copy link
Contributor

jkowall commented Jul 8, 2022

@jkowall Any particular reason for thumbs downing this draft PR?

Nope, meant to thumbs up! Good contribution.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @davidji99.

Typically, please open an issue proposing the changes you'd like to see.

Overall this looks good to me, but there are quite a few minor details to work through.

I'd also like to see these metrics represented in the results of scraper_test.go and integration_test.go, or understand why not.

receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
@davidji99
Copy link
Contributor Author

Thanks for the contribution @davidji99.

Typically, please open an issue proposing the changes you'd like to see.

Overall this looks good to me, but there are quite a few minor details to work through.

I'd also like to see these metrics represented in the results of scraper_test.go and integration_test.go, or understand why not.

Thanks for the review! I will work through your proposed changes.

Yes, I plan on adding these new metrics to the relevant _test.go files.

@davidji99 davidji99 force-pushed the add-more-elasticsearch-metrics branch 2 times, most recently from bac4f06 to 28392e3 Compare July 12, 2022 11:43
@davidji99
Copy link
Contributor Author

Hi @djaglowski,

I've updated receiver/elasticsearchreceiver/testdata/sample_payloads/nodes_linux.json and receiver/elasticsearchreceiver/testdata/expected_metrics/full.json to incorporate the new metrics. However, when running the TestScraper test, I consistently get:

=== CONT  TestScraper
    scraper_test.go:61:
        	Error Trace:	/Users/david.ji/dev_exclusions/opentelemetry-collector-contrib/receiver/elasticsearchreceiver/scraper_test.go:61
        	Error:      	Received unexpected error:
        	            	number of metrics does not match expected: 41, actual: 32

Maybe I'm missing something here but I expected the metric count to be the same?

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

My suggestion for updating the expected metric json files is to temporarily insert a golden.WriteMetrics(expectedFile, actualMetrics) call in the test code. This will overwrite the expected file with the metrics you actually get from the scraper. Then, take a close look at the diff and see if it makes sense at that point.

receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
@davidji99 davidji99 force-pushed the add-more-elasticsearch-metrics branch 2 times, most recently from f30e658 to b78fdda Compare July 13, 2022 02:41
@davidji99
Copy link
Contributor Author

Thank you for the tip @djaglowski. I've updated this PR with passing tests. Could you take another look?

receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/elasticsearchreceiver/documentation.md Outdated Show resolved Hide resolved
@davidji99 davidji99 force-pushed the add-more-elasticsearch-metrics branch from 1a84875 to 5e49460 Compare July 14, 2022 01:43
@davidji99 davidji99 force-pushed the add-more-elasticsearch-metrics branch from 5e49460 to a6e09ba Compare July 14, 2022 01:55
@davidji99
Copy link
Contributor Author

@djaglowski Updated the PR. Can you re-run tests?

@djaglowski
Copy link
Member

Thanks for the contribution @davidji99!

@djaglowski djaglowski merged commit 1908487 into open-telemetry:main Jul 14, 2022
@davidji99 davidji99 deleted the add-more-elasticsearch-metrics branch July 14, 2022 20:38
@davidji99
Copy link
Contributor Author

Thank you for your help and patience @djaglowski !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants