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

Refactor prometheus endpoint parsing to look similar to upstream prometheus #6332

Merged
merged 2 commits into from
Feb 18, 2018

Conversation

vjsamuel
Copy link
Contributor

@vjsamuel vjsamuel commented Feb 9, 2018

I have changed the prometheus helper to parse in a manner similar to how it is done upstream. This would make it easier to know if a prometheus endpoint is wrong or if the issue comes from our Beats parsing logic.

Upstream sample link: https://github.com/prometheus/common/blob/master/expfmt/bench_test.go

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@@ -65,6 +65,7 @@ func (m *MetricSet) Fetch() ([]common.MapStr, error) {
return nil, fmt.Errorf("Unable to decode response from prometheus endpoint")
}

fmt.Println(families)
Copy link
Member

Choose a reason for hiding this comment

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

leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

@ruflin
Copy link
Member

ruflin commented Feb 9, 2018

@vjsamuel Do updating prometheus dependencies require the change or the other way around? If not I suggest we update the dependency in a separate PR first?

@vjsamuel
Copy link
Contributor Author

vjsamuel commented Feb 9, 2018

@ruflin the vendor update and this patch are independent. i have removed the vendor update. i will submit another PR now.

@exekias
Copy link
Contributor

exekias commented Feb 9, 2018

ok to test

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

LGTM

@vjsamuel
Copy link
Contributor Author

vjsamuel commented Feb 9, 2018

@exekias please do not merge. i am still figuring something out on this.

@ruflin ruflin added the in progress Pull request is currently in progress. label Feb 12, 2018
@vjsamuel
Copy link
Contributor Author

@exekias i ended up changing only a minor error handling as per how prometheus client does it. please merge once CI approves and when you are ok with the change.

@@ -224,6 +224,7 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di
- Docker and Kubernetes modules are now GA, instead of Beta. {pull}6105[6105]
- Add pct calculated fields for Pod and container CPU and memory usages. {pull}6158[6158]
- Add statefulset support to Kubernetes module. {pull}6236[6236]
- Refactor prometheus endpoint parsing to look similar to upstream prometheus {pull}6332[6332]
Copy link
Member

Choose a reason for hiding this comment

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

When I initially read this I thought the parsing of prometheus input -> elasticsearch output changed. But it seems mainly the error handling changed / was improved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was always meant to change only the way parsing happens.

@ruflin ruflin merged commit 819959c into elastic:master Feb 18, 2018
@vjsamuel vjsamuel deleted the prometheus_parse_refactor branch March 5, 2018 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Pull request is currently in progress. Metricbeat Metricbeat review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants