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

Elasticsearch exporter integration #347

Merged
merged 10 commits into from
Jan 22, 2021
Merged

Conversation

colega
Copy link
Contributor

@colega colega commented Jan 21, 2021

PR Description

Embeds the elasticsearch exporter

Which issue(s) this PR fixes

Uh. Oh! There's no issue for this.

Notes to the Reviewer

First commit is a refactor that I'd recommend to review separately.

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@CLAassistant
Copy link

CLAassistant commented Jan 21, 2021

CLA assistant check
All committers have signed the CLA.

@colega
Copy link
Contributor Author

colega commented Jan 21, 2021

I'm not sure how we test exporters 🤔
I could mock the response from the ES and run the integration in the test.

Refactor the CollectorIntegartion to support multiple collectors and an
optional runner function (not used in this commit).

This changes will be used in the oncoming ElasticSearch integration.
Vendored github.com/justwatchcom/elasticsearch_exporter

This will be used in conjunction with grafana/jsonnet-libs#427
@colega colega force-pushed the elasticsearch-exporter-integration branch from a709890 to 34913c4 Compare January 21, 2021 17:59
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Nice, thanks for working on this! I like the changes to the generic CollectorIntegration.

Just a few small pieces of feedback, but this should be good to go soon!

Can you add this to the testing environment? Look at the example integrations config that enables every integration and the docker compose environment that runs the SUOs

docs/configuration-reference.md Show resolved Hide resolved
docs/configuration-reference.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@colega
Copy link
Contributor Author

colega commented Jan 22, 2021

@rfratto all feedback addressed, I've added it to the configs here: bfb9e65

BTW, for the record, this exporter exposes 338 metrics with the default config:

$ curl -s localhost:12345/integrations/elasticsearch_exporter/metrics | grep -v -E "^#" -c
338

Ready to be reviewed again.

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all your work!

.github/depcheck.yml Outdated Show resolved Hide resolved
@rfratto rfratto merged commit fc77062 into master Jan 22, 2021
@rfratto rfratto deleted the elasticsearch-exporter-integration branch January 22, 2021 13:05
@mattdurham mattdurham mentioned this pull request Sep 7, 2021
3 tasks
mattdurham pushed a commit that referenced this pull request Nov 11, 2021
* Make CollectorIntegration more flexible

Refactor the CollectorIntegartion to support multiple collectors and an
optional runner function (not used in this commit).

This changes will be used in the oncoming ElasticSearch integration.

* ElasticSearch exporter integration

Vendored github.com/justwatchcom/elasticsearch_exporter

This will be used in conjunction with grafana/jsonnet-libs#427

* Remove `es.` prefixes from ElasicSearch configs

And make consistent usage of underscores in the yaml keys.

* Fix the docs, add elasticsearch_exporter reference

* Remove unneeded `_ =` in elasticsearch_exporter

* Credits to @colega for elasticsearch_exporter

* Consistent ES exporter config with other exporters

Move the init(), use pointer receivers.

* Add elasticsearch_expoter example

Also renamed elasticsearch `uri` config key to `address`.

* Add elasticsearch_exporter to depcheck.yml

* only reference elasticsearch_exporter once in depcheck file

Co-authored-by: Robert Fratto <robertfratto@gmail.com>
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Apr 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants