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

Index level metrics #131

Merged
merged 2 commits into from Nov 1, 2018
Merged

Index level metrics #131

merged 2 commits into from Nov 1, 2018

Conversation

lukas-vlcek
Copy link
Collaborator

Adding esplugin integaration tests.
Bring indices level metrics back.

@lukas-vlcek
Copy link
Collaborator Author

@vvanholl PTAL

I need your review. I do not like creating massive PRs. The next feature I will contribute is #124 and it would be really great if this PR is already merged before I submit next one. We also need back-port these changes into older releases. So there is a lot of work to do...

Let me sum up what this PR brings:

  1. Adoption of gradle esplugin. This simplifies ES plugin build automation, streamlines gradle build script (i.e plugin descriptor and Java security manager configuration) and enables implementation of REST API tests using Elasticsearch tools.

  2. Added couple of basic REST API tests.

  3. (Re-)Introducing index level metrics. I was also able to clean up the original code and drop some of the unnecessary ES plugin boilerplate code. I added complete coverage of all index level metrics into REST API tests. Although this sounds unnecessary I was able to spot and fix at least two minor issues in the metrics (one was typo in metric name, second was registration of metrics that were no longer available and reported).

In general I think it will be useful to add REST tests for every exposed metric. In the future I want to make sure that there is also Grafana dashboard available for each release version of this plugin. For this I want to be sure all the used metric names match.

@lukas-vlcek
Copy link
Collaborator Author

lukas-vlcek commented Nov 1, 2018

Just note: if you wan to run all tests use gradle clean check

@vvanholl
Copy link
Owner

vvanholl commented Nov 1, 2018

As usual this is a very awesome contribution to my project :)
Thank you for your work !

Vincent.

By the way, from now CI will also done by Gitlab CI.

@vvanholl vvanholl merged commit 365a3b7 into vvanholl:master Nov 1, 2018
@lukas-vlcek lukas-vlcek deleted the index_level_metrics branch November 2, 2018 12:02
@lukas-vlcek
Copy link
Collaborator Author

lukas-vlcek commented Nov 2, 2018

By the way, from now CI will also done by Gitlab CI.

This is great news! I am trying to learn more about Gitlab CI ATM.
https://gitlab.com/vvanholl/elasticsearch-prometheus-exporter/

Does the CI start when PR is created? I created #132 but it does not seem to kickstart CI. Is there anything we can do about this?

Also it would be nice to add project badges to README.dm.

I was able to figure out build badge for the master branch:

https://gitlab.com/vvanholl/elasticsearch-prometheus-exporter/badges/master/build.svg

Though coverage badge does not seem to work for me:

https://gitlab.com/vvanholl/elasticsearch-prometheus-exporter/badges/master/coverage.svg

I need to dig deeper. Thoughts welcome.

@lukas-vlcek
Copy link
Collaborator Author

Hmm... is it possible that Gitlab-CI does not support easy pipeline start when new PR is created in GitHub? See https://stackoverflow.com/questions/43918324/trigger-jobs-in-gitlab-ci-on-merge-request

@vvanholl
Copy link
Owner

vvanholl commented Nov 2, 2018

Unfortunately this is not yet implemented as well : https://gitlab.com/gitlab-org/gitlab-ce/issues/23902

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.

None yet

2 participants