-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Conversation
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? |
e8051b3
to
f90ccd9
Compare
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
@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? |
f90ccd9
to
e1fa624
Compare
@ruflin the vendor update and this patch are independent. i have removed the vendor update. i will submit another PR now. |
ok to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@exekias please do not merge. i am still figuring something out on this. |
e1fa624
to
62a66a9
Compare
62a66a9
to
f777f5b
Compare
@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] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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