Fix for queue name mismatch for metric collection #4220
Conversation
The message being placed on the queue to kick off metric collection was using the ems class name to determine the queue name. The metric collector workers use a hard-coded value for the queue name that would not match the ems class. Added a class method to each ems which pulls the correct queue name from the worker class when enqueueing a message for metric collection. Fixes ManageIQ#4201
| @@ -16,7 +16,7 @@ def queue_name_for_metrics_collection | |||
| raise "Unsupported type #{self.class.name} (id: #{id})" | |||
| end | |||
|
|
|||
| ems.class.name[3..-1].underscore | |||
| ems.class.metrics_collect_queue_name | |||
There was a problem hiding this comment.
you may want to have EMS have an instance method that calls the class method, so that the caller has ems.metrics_collect_queue_name
|
Checked commit carbonin@cd0893a with ruby 1.9.3, rubocop 0.33.0, and haml-lint 0.13.0 |
|
LGTM 👍 |
| @@ -40,6 +40,10 @@ def self.default_blacklisted_event_names | |||
| ) | |||
| end | |||
|
|
|||
| def self.metrics_collect_queue_name | |||
| MetricsCollectorWorker.default_queue_name | |||
There was a problem hiding this comment.
I'd rather see this pushed up to ManageIQ::Providers::BaseManager (and renamed to metrics_collector_queue_name).
class BaseManager
def self.metrics_collector_queue_name
self::MetricsCollectorWorker.default_queue_name
end
endThen, this is only needed in one place and all of the CloudManagers and InfraManagers and OtherManagers will get it for free.
There was a problem hiding this comment.
Will that cause any issues with providers that inherit from ManageIQ::Providers::BaseManager but don't have a self::MetricsCollectorWorker? The only one I could find right now is ManageIQ::Providers::Microsoft::InfraManager
There was a problem hiding this comment.
As the code is right now it shouldn't get called for those providers, but we will be opening up a method that we know will always fail.
There was a problem hiding this comment.
I tried this locally in rails console and ManageIQ::Providers::Microsoft::InfraManager.metrics_collector_queue_name just returns nil.
I think that's safe. Otherwise, we could have the impl in the base MetricsCollectorWorker return empty string instead to avoid NPEs.
There was a problem hiding this comment.
@carbonin OK not pretty but we could reconsider the idea we had of:
if ems.respond_to?(:metrics_collector_queue_name)
ems.metrics_collect_queue_name
else
ems.class.name[3..-1].underscore
end
|
This is a good find! 👍 Couple suggestions above. |
|
Nice job getting this fixed! 👏 |
|
👍 |
1 similar comment
|
👍 |
Fix for queue name mismatch for metric collection
The message being placed on the queue to kick off metric
collection was using the ems class name to determine the
queue name.
The metric collector workers use a hard-coded value for
the queue name that would not match the ems class.
Added a class method to each ems which pulls the correct
queue name from the worker class when enqueueing a message
for metric collection.
Fixes #4201