-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add support for reading prometheus endpoint #20
Conversation
@miq-bot add_label compute/containers, wip cc// @simon3z @cben @moolitayer @zeari |
@yaacov Cannot apply the following label because they are not recognized: compute/containers |
9e3e6d7
to
849ade5
Compare
@yaacov let's split this in 3 parts (2 of which we can actually merge):
|
Moved to #25
Implementation depend on #25. |
@yaacov I'd rather not have the new metrics depending on a refactor if possible. I want to backport them in the cleanest way possible. |
This pull request is not mergeable. Please rebase and repush. |
849ade5
to
bc1f8a6
Compare
ac2367f
to
b0b08f7
Compare
8791308
to
b3b8ced
Compare
This pull request is not mergeable. Please rebase and repush. |
b3b8ced
to
470b865
Compare
d2f0a86
to
d9e1450
Compare
@yaacov should we add the prometheus spec tests as well? |
@simon3z 👍 I will |
8a023e9
to
c17ccbc
Compare
c17ccbc
to
afd4bb8
Compare
Some comments on commit yaacov@afd4bb8 spec/models/manageiq/providers/kubernetes/container_manager/metrics_capture_spec.rb
|
Checked commit yaacov@afd4bb8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@@ -57,7 +58,11 @@ def perf_collect_metrics(interval_name, start_time = nil, end_time = nil) | |||
"[#{start_time}] [#{end_time}]") | |||
|
|||
begin | |||
context = CaptureContext.new(target, start_time, end_time, INTERVAL) | |||
context = if ems && ems.connection_configurations.prometheus.try(:endpoint) |
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.
you defined a method called prometheus_endpoint. Can you use that?
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.
no we can not use it here :-(
it's a chicken and egg problem, to use it we need a context, but if we do not have a context we can't use it :-(
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.
So move it to the manager and the capture context
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.
moving functionality around just for the sake of moving it is pointless, is their a reason for moving it ?
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.
you clearly need this function in a wider scope then the capture context (for example to determine which context to create) then you won't have to copy it in different places.
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.
(for example to determine which context to create)
you give this case as an example for wider scope ?
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.
- There is something which I don't get, maybe you could explain. metrics collection is supposed to be disabled unless we have an endpoint named hawkular due to https://github.com/ManageIQ/manageiq-providers-kubernetes/blob/master/app/models/manageiq/providers/kubernetes/container_manager/metrics_collector_worker.rb#L18
- This change is hard to review since it adds new functionality and also renames a bunch of files. Please split it to ease reviewers work - one for refactor related stuff (moving stuff around, renaming etc) and one for new functionality
Thanks!
We already tried that [1] I do not think we should do that again. [1] #25 |
than we should fix the |
I'm not asking that to create extra work for you. |
I did not think so :-) it's a matter of time usage and priorities -
New functionality, especially of this size, will come with bugs, and we will fix them when we find them -
[1] ManageIQ/manageiq-ui-classic#1677 |
Yes, but: |
#59 fixes the |
@yaacov I've asked to move a few files to a different PR so that review is more effective - I do it all the time. I will not approve this pull request as it is. |
Ok :-) |
Description
Add support for reading Prometheus endpoint
If prometheus endpoint exist, read metrics from prometeus