Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adjust LiveMetric conditions parsing #10570

Merged
merged 2 commits into from
Aug 22, 2016
Merged

Adjust LiveMetric conditions parsing #10570

merged 2 commits into from
Aug 22, 2016

Conversation

lucasponce
Copy link
Contributor

@lucasponce lucasponce commented Aug 17, 2016 •
edited by miq-bot
Loading

Fix needed to resolve #10087.
After changes on AAArM conditions are wrapped into an array that needs minor adjustment on live_metrics side.

@lucasponce
Copy link
Contributor Author

@miq-bot add_label bug,providers/hawkular

@lucasponce lucasponce force-pushed the fix-livemetrics-change-conditions-format branch from 50d9ec5 to 6fb3ec7 Compare August 17, 2016 22:35
@lucasponce
Copy link
Contributor Author

/cc @abonas @pilhuhn

@lucasponce lucasponce force-pushed the fix-livemetrics-change-conditions-format branch from 6fb3ec7 to f348765 Compare August 18, 2016 08:02
@lucasponce
Copy link
Contributor Author

lucasponce commented Aug 18, 2016 •
edited
Loading

Added a new commit related to the same context.
I think it makes sense to put it on this PR to not leave this fix incomplete.
Cols were not filtered correctly for LiveMetric class due get_include_for_find expects the virtual column is previously created.
LiveMetric has one static column (timestamp) but rest of the columns are generated dynamically.
This PR fixes that and it is transparent to other uses.

@lucasponce lucasponce force-pushed the fix-livemetrics-change-conditions-format branch from 5950c00 to 1c88b14 Compare August 18, 2016 11:05
@lucasponce
Copy link
Contributor Author

Following this PR I have identified two potential enhancements to improve the performance of Hawkular calls, so once it is merged I will work on that.

chessbyte
chessbyte reviewed Aug 21, 2016
View reviewed changes
app/models/live_metric.rb
@@ -43,7 +42,7 @@ def self.parse_condition(column, op, value)
end

def self.process_conditions(conditions)
parsed_conditions = parse_conditions(conditions)
parsed_conditions = parse_conditions(conditions[0])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conditions.first is more idiomatic than conditions[0]

@lucasponce lucasponce force-pushed the fix-livemetrics-change-conditions-format branch from 1c88b14 to 39cdbb4 Compare August 21, 2016 17:57
@lucasponce
Copy link
Contributor Author

Perhaps the two commits can be squashed into one.
Let me know if you want that format for this PR.

@lucasponce lucasponce force-pushed the fix-livemetrics-change-conditions-format branch from 39cdbb4 to 8e73992 Compare August 21, 2016 18:03
@miq-bot
Copy link
Member

miq-bot commented Aug 21, 2016

Checked commits lucasponce/manageiq@f348765~...8e73992 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
3 files checked, 0 offenses detected
Everything looks good. 🏆

@abonas
Copy link
Member

abonas commented Aug 21, 2016

Perhaps the two commits can be squashed into one.
Let me know if you want that format for this PR.

I think it's ok the way it is now.
@miq-bot assign @chessbyte

@miq-bot miq-bot assigned chessbyte and unassigned abonas Aug 21, 2016
@lucasponce
Copy link
Contributor Author

Some errors on travis
https://travis-ci.org/ManageIQ/manageiq/jobs/153969379#L1084
but those are not related to this PR.

@lucasponce lucasponce closed this Aug 22, 2016
@lucasponce lucasponce reopened this Aug 22, 2016
@lucasponce
Copy link
Contributor Author

I have close/reopened this PR to trigger travis CI.

@abonas
Copy link
Member

abonas commented Aug 22, 2016

@chessbyte this is super green 🍏

@chessbyte chessbyte merged commit 3316c1d into ManageIQ:master Aug 22, 2016
@chessbyte chessbyte added this to the Sprint 45 Ending Aug 22, 2016 milestone Aug 22, 2016
@lucasponce lucasponce deleted the fix-livemetrics-change-conditions-format branch August 23, 2016 07:45
@kbrock kbrock mentioned this pull request Jan 24, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@chessbyte chessbyte

Labels
bug providers/middleware
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Monitoring->Utilization view hangs for middleware servers and middleware datasources
4 participants
@lucasponce @miq-bot @abonas @chessbyte