Add a queue metrics capture method to container manager #206
Conversation
yaacov
force-pushed
the
add-a-queue_metrics_capture-method-to-container-manager
branch
from
5a6b780 to
b5d7103
Compare
|
@miq-bot add_label bug, metrics, gaprindashvili/yes @cben @moolitayer @zeari @Ladas please review |
yaacov
force-pushed
the
add-a-queue_metrics_capture-method-to-container-manager
branch
from
b5d7103 to
096d2c3
Compare
| begin | ||
| target.perf_capture_queue(interval_name, options) | ||
| rescue StandardError => err | ||
| _log.warn("Failed to queue perf_capture for target [#{target.class.name}], [#{target.id}], [#{target.name}]: #{err}") |
There was a problem hiding this comment.
👍 perf_capture_queue may raise exceptions, especially ArgumentError.
However, I expect in most cases where one target raises, all would raise,
so perhaps this function should just let exception propogate?
[Your call, I dont have opinion here]
There was a problem hiding this comment.
yeah we should let it propagate and rather do _log.error, there shouldn't be a case where only 1 fails
yaacov
force-pushed
the
add-a-queue_metrics_capture-method-to-container-manager
branch
2 times, most recently
from
114804c to
3401100
Compare
| @@ -266,4 +266,17 @@ def evaluate_alert(_alert_id, _event) | |||
| # and these should automatically be translated to alerts. | |||
| true | |||
| end | |||
|
|
|||
| def queue_metrics_capture(interval_name = 'realtime', options = {}) | |||
There was a problem hiding this comment.
Since this is meant for queuing in the UI I would prefer if you remove both parameters
|
|
||
| targets.each do |target| | ||
| begin | ||
| target.perf_capture_queue(interval_name, options) |
There was a problem hiding this comment.
See Metric::CiMixin::Capture.perf_capture_realtime_now
can you use that here?
yaacov
force-pushed
the
add-a-queue_metrics_capture-method-to-container-manager
branch
from
3401100 to
46d118d
Compare
|
Checked commit yaacov@46d118d with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
Update: yaacov and myself observed we have some errors in the log post queue_metrics_capture calls. We also observed some cases where the end_time - start_time for metrics collection is very large ( |
This errors have no connection to this PR.
No, the are related to
Regular 'historical' requsts ask fot different time slots depending on gaps and missing data, it's ok to have strange :-) time slots, when queue by system worker. |
| targets.each do |target| | ||
| begin | ||
| target.perf_capture_queue('realtime', :priority => MiqQueue::HIGH_PRIORITY) | ||
| rescue StandardError => err |
There was a problem hiding this comment.
[cosmetic, LGTM either way] since you now re-raise, you could move this rescue to top level of the function, saving begin..end.
There was a problem hiding this comment.
Ahh, I want to have the target that raised the error, If I move the rescue to top level I lose this info :-(
There was a problem hiding this comment.
ah, right. remind me to resist making completely non-important code comments :)
|
Removed |
Description
Add a method to queue reading of metrics.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1527877
Target: 5.9.1