-
Notifications
You must be signed in to change notification settings - Fork 60
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
Disable metrics and events in unsupported regions #36
Conversation
@miq-bot add_label wip |
@djberg96 Cannot apply the following label because they are not recognized: wip |
Note to self: as per conversation with the UI folks, we may want to add a textual summary for the UI at app/helpers/ems_cloud_helper/textual_summary.rb (in the classic-ui repo). |
@miq-bot remove_label wip |
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.
Overall looks good and straight forward.
Personally I'd split this into 4 PRs
insights?
:timeline
- metrics
- supports specs for discovery, etc
This makes it easier to backport things in the future.
But not really important
@@ -1,3 +1,5 @@ | |||
require 'azure-armrest' |
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.
wont this already be required by connect
?
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.
I was hitting some kind of weird load ordering issue that I couldn't nail down, which resulted in undefined constant errors.
@@ -183,6 +183,13 @@ def raw_metrics_for_counter(storage_conn, counter, start_time, end_time) | |||
end | |||
|
|||
def get_counters(metrics_conn) | |||
ems = target.ext_management_system | |||
|
|||
unless ems.supports_timeline? |
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.
would this be better ems.insights?
to not conflate with timelines / events ?
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.
Yes, probably. I will update.
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.
Just one question/suggestion, but this is great.
@@ -183,6 +183,13 @@ def raw_metrics_for_counter(storage_conn, counter, start_time, end_time) | |||
end | |||
|
|||
def get_counters(metrics_conn) | |||
ems = target.ext_management_system | |||
|
|||
unless ems.supports_timeline? |
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.
@djberg96 - I would probably move this to line 78 after we check for an EMS, this would keep the checks together and avoid calling methods if metrics isn't supported. Or is there a reason for placing this line here?
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.
I'll try it and report back.
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.
@bronaghs Ok, I've moved it as you suggested. Seems to work just as well there.
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.
@bronaghs Upon further review it seems to cause test failures because of with_provider_connection which is called again later within the same method. I've moved it back for now.
Update get_counter to skip counter collection if timeline isn't supported. Added supports feature specs. Initial commit.
Checked commits https://github.com/djberg96/manageiq-providers-azure/compare/98c982ddb15d0ac60e0a3b70761ab1bde61c9bd4~...8d18453d847ba408a5876e6c9482c95091f8288f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
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.
👍 from me
@bronaghs do you want to merge?
This PR uses the SupportsFeatureMixin and the most recent azure-armrest gem to disable metrics and events collection in regions that are not supported.
Note that this requires the addition of
:metrics
to the support matrix. See ManageIQ/manageiq#13381You will also have to update gems-pending-config to point to azure-armrest 0.6.0.