Add faster MiqReportResult helper methods for viewing saved report results #17590

NickLaMuro · 2018-06-14T21:42:44Z

Adds two methods:

MiqReportResult#has_report?
MiqReportResult#contains_records?

This is specifically used to avoid having to fetch and deserialize the binary_blob associated with the MiqReportResult, which has the entire Ruport::Data::Table object saved in it, and eventually is never used in some of the controllers that make use of them.

`MiqReportResult#has_report?`

A quicker check than having to use MiqReportResult#report_results, since binary_blob doesn't need to be loaded into memory. On top of that, generally the binary_blob.data isn't even used in the controller at the end of the day, and this version of the MiqReport record ends up being what is used when generating the page for viewing.

`MiqReportResult#contains_records?`

Like MiqReportResult#has_report?, this is a faster version of checking if the report_result has rows.

Like MiqReport#contains_records?, this will check against the extras attribute on the deserialized version of the report column, but instead of using the table value (which is not saved into the report column), just do a light SQL existence check on the html_details to confirm there are generated rows for this report.

The SQL query most likely won't be needed, and it is super light weight, only returning a single digit, or an empty result set if there are no records.

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1590908
Associated UI PR to use these methods: Faster report result index pages manageiq-ui-classic#4143

Will put benchmarks in the associated UI PR.

These are new methods added in: ManageIQ/manageiq#17590 And are for more efficient for the use cases of ChargebackController and ReportController::SavedReports.

jrafanie · 2018-06-15T14:25:30Z

app/models/miq_report_result.rb

+  # Use this method over `report_results` if you don't plan on using the
+  # `binary_blob` associated with the MiqReportResult (chances are you don't
+  # need it).
+  def has_report?


I like the cop suggestion of report? What do you think ?

Honestly, I was thinking about this and want to make this "more verbose". report?, and even has_report? gives the impression that report is a relation, not a column on this report. I am almost thinking of renaming this to valid_report_column? or something like that.

Thoughts?

hmm, good point... why would report not be a MiqReport? It seems like the report result might be useless without a report object... but I'm not sure how it's used if there isn't a report. Maybe linked_report? since the UI use case you changed is to change the selected node in the chargeback tree for the report corresponding to this report result.

Again, this is a serialized report shoved into the report column, so it could be serialized to anything technically speaking.

linked_report? to me still sounds like we are talking about a relation, when in fact, this is part of the record itself. Does that rational make sense?

jrafanie · 2018-06-15T14:26:48Z

Kicked travis. Other than the method naming, I'm good with these changes.

jrafanie · 2018-06-15T14:29:10Z

@NickLaMuro you might want to put the bz link in the commits so the bz becomes self documenting when the commits get merged and copied to the bz automagically by the bot. I document my commits as if they are going directly to the BZ so testing/recreation steps are all there.

NickLaMuro · 2018-06-15T14:55:47Z

@NickLaMuro you might want to put the bz link in the commits so the bz becomes self documenting when the commits get merged and copied to the bz automagically by the bot.

Yeah, that makes sense I guess. Will do.

https://bugzilla.redhat.com/show_bug.cgi?id=1590908 These are new methods added in: ManageIQ/manageiq#17590 And are for more efficient for the use cases of ChargebackController and ReportController::SavedReports.

jrafanie · 2018-06-18T16:10:36Z

@NickLaMuro any idea why this is failing the chargeback tests? Are they calling these new methods???

NickLaMuro · 2018-06-18T16:11:38Z

@jrafanie Unsure, but when I looked, it seemed like the test failures were benign and unrelated. I will look again though, just to be sure.

https://bugzilla.redhat.com/show_bug.cgi?id=1590908 A quicker check than having to use `MiqReportResult#report_results`, since binary_blob doesn't need to be loaded into memory. On top of that, generally the binary_blob.data isn't even used in the controller at the end of the day, and this version of the MiqReport record ends up being what is used when generating the page for viewing.

https://bugzilla.redhat.com/show_bug.cgi?id=1590908 Like `MiqReportResult#has_report?`, this is a faster version of checking if the report_result has rows. Like `MiqReport#contains_records?`, this will check against the `extras` attribute on the deserialized version of the `report` column, but instead of using the `table` value (which is not saved into the `report` column), just do a light SQL existence check on the `html_details` to confirm there are generated rows for this report. The SQL query most likely won't be needed, and it is super light weight, only returning a single digit, or an empty result set if there are no records.

miq-bot · 2018-06-18T16:22:32Z

Checked commits NickLaMuro/manageiq@6b2e57d~...3a26db0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

jrafanie · 2018-06-18T19:05:16Z

@NickLaMuro can you add labels to match the other one for the bz?

NickLaMuro · 2018-06-18T19:07:56Z

@miq-bot blocker, gaprindashvili/yes

miq-bot · 2018-06-18T19:08:03Z

@NickLaMuro unrecognized command 'blocker', ignoring...

Accepted commands are: add_label, add_reviewer, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

NickLaMuro · 2018-06-18T19:09:29Z

...

@miq-bot add_label blocker, gaprindashvili/yes

https://bugzilla.redhat.com/show_bug.cgi?id=1590908 These are new methods added in: ManageIQ/manageiq#17590 And are for more efficient for the use cases of ChargebackController and ReportController::SavedReports.

https://bugzilla.redhat.com/show_bug.cgi?id=1590908 These are new methods added in: ManageIQ/manageiq#17590 And are far more efficient for the use cases of ChargebackController and ReportController::SavedReports.

jrafanie · 2018-06-19T14:35:05Z

@miq-bot add_label fine/yes

Add faster MiqReportResult helper methods for viewing saved report results (cherry picked from commit dbf6596) https://bugzilla.redhat.com/show_bug.cgi?id=1594387

simaishi · 2018-06-22T19:38:31Z

Fine backport details:

$ git log -1
commit d1177fd98c4f3fbc1825c9a432bb92017bc99d6e
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Mon Jun 18 15:05:17 2018 -0400

    Merge pull request #17590 from NickLaMuro/faster_saved_report_methods
    
    Add faster MiqReportResult helper methods for viewing saved report results
    (cherry picked from commit dbf6596ca6903a4c7d30cc16fe9f137db9a6a026)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1594387

Add faster MiqReportResult helper methods for viewing saved report results (cherry picked from commit dbf6596) https://bugzilla.redhat.com/show_bug.cgi?id=1594386

simaishi · 2018-06-22T19:45:22Z

Gaprindashvili backport details:

$ git log -1
commit ee76f422c42f513122f0a379ea8c70bc65804cef
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Mon Jun 18 15:05:17 2018 -0400

    Merge pull request #17590 from NickLaMuro/faster_saved_report_methods
    
    Add faster MiqReportResult helper methods for viewing saved report results
    (cherry picked from commit dbf6596ca6903a4c7d30cc16fe9f137db9a6a026)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1594386

https://bugzilla.redhat.com/show_bug.cgi?id=1590908 These are new methods added in: ManageIQ/manageiq#17590 And are far more efficient for the use cases of ChargebackController and ReportController::SavedReports.

https://bugzilla.redhat.com/show_bug.cgi?id=1594386 Gaprindashvili Backport commit from: ManageIQ#4143 Original SHA: ff7ee24 Rest of original message below... * * * These are new methods added in: ManageIQ/manageiq#17590 And are far more efficient for the use cases of ChargebackController and ReportController::SavedReports.

https://bugzilla.redhat.com/show_bug.cgi?id=1594387 Gaprindashvili Backport commit from: ManageIQ#4143 Original SHA: ff7ee24 Rest of original message below... * * * These are new methods added in: ManageIQ/manageiq#17590 And are far more efficient for the use cases of ChargebackController and ReportController::SavedReports.

…rtResult https://bugzilla.redhat.com/show_bug.cgi?id=1594387 Gaprindashvili Backport commit from: ManageIQ#4143 Original SHA: ff7ee24 Rest of original message below... * * * These are new methods added in: ManageIQ/manageiq#17590 And are far more efficient for the use cases of ChargebackController and ReportController::SavedReports.

…tResult https://bugzilla.redhat.com/show_bug.cgi?id=1594386 Gaprindashvili Backport commit from: ManageIQ#4143 Original SHA: ff7ee24 Rest of original message below... * * * These are new methods added in: ManageIQ/manageiq#17590 And are far more efficient for the use cases of ChargebackController and ReportController::SavedReports.

NickLaMuro mentioned this pull request Jun 14, 2018

Faster report result index pages ManageIQ/manageiq-ui-classic#4143

Merged

jrafanie reviewed Jun 15, 2018

View reviewed changes

NickLaMuro force-pushed the faster_saved_report_methods branch from e567768 to b321ff5 Compare June 15, 2018 16:46

NickLaMuro force-pushed the faster_saved_report_methods branch from b321ff5 to b8293ff Compare June 18, 2018 16:18

NickLaMuro force-pushed the faster_saved_report_methods branch from b8293ff to 3a26db0 Compare June 18, 2018 16:19

jrafanie approved these changes Jun 18, 2018

View reviewed changes

jrafanie added core/reporting enhancement labels Jun 18, 2018

jrafanie added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 18, 2018

jrafanie merged commit dbf6596 into ManageIQ:master Jun 18, 2018

jrafanie self-assigned this Jun 18, 2018

miq-bot added blocker gaprindashvili/yes labels Jun 18, 2018

miq-bot added the fine/yes label Jun 19, 2018

simaishi added fine/backported and removed fine/yes labels Jun 22, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jun 22, 2018

NickLaMuro mentioned this pull request Jun 25, 2018

[GAPRINDASHVILI] Faster report result index pages ManageIQ/manageiq-ui-classic#4204

Merged

NickLaMuro mentioned this pull request Jun 25, 2018

[FINE] Faster report result index pages ManageIQ/manageiq-ui-classic#4208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add faster MiqReportResult helper methods for viewing saved report results #17590

Add faster MiqReportResult helper methods for viewing saved report results #17590

NickLaMuro commented Jun 14, 2018 •

edited

Loading

jrafanie Jun 15, 2018

NickLaMuro Jun 15, 2018

jrafanie Jun 15, 2018

NickLaMuro Jun 15, 2018

jrafanie commented Jun 15, 2018

jrafanie commented Jun 15, 2018

NickLaMuro commented Jun 15, 2018

jrafanie commented Jun 18, 2018

NickLaMuro commented Jun 18, 2018 •

edited

Loading

miq-bot commented Jun 18, 2018

jrafanie commented Jun 18, 2018

NickLaMuro commented Jun 18, 2018

miq-bot commented Jun 18, 2018

NickLaMuro commented Jun 18, 2018

jrafanie commented Jun 19, 2018

simaishi commented Jun 22, 2018

simaishi commented Jun 22, 2018

Add faster MiqReportResult helper methods for viewing saved report results #17590

Add faster MiqReportResult helper methods for viewing saved report results #17590

Conversation

NickLaMuro commented Jun 14, 2018 • edited Loading

MiqReportResult#has_report?

MiqReportResult#contains_records?

Links

jrafanie Jun 15, 2018

Choose a reason for hiding this comment

NickLaMuro Jun 15, 2018

Choose a reason for hiding this comment

jrafanie Jun 15, 2018

Choose a reason for hiding this comment

NickLaMuro Jun 15, 2018

Choose a reason for hiding this comment

jrafanie commented Jun 15, 2018

jrafanie commented Jun 15, 2018

NickLaMuro commented Jun 15, 2018

jrafanie commented Jun 18, 2018

NickLaMuro commented Jun 18, 2018 • edited Loading

miq-bot commented Jun 18, 2018

jrafanie commented Jun 18, 2018

NickLaMuro commented Jun 18, 2018

miq-bot commented Jun 18, 2018

NickLaMuro commented Jun 18, 2018

jrafanie commented Jun 19, 2018

simaishi commented Jun 22, 2018

simaishi commented Jun 22, 2018

NickLaMuro commented Jun 14, 2018 •

edited

Loading

`MiqReportResult#has_report?`

`MiqReportResult#contains_records?`

NickLaMuro commented Jun 18, 2018 •

edited

Loading