Foreman 2.2.2 - Edit "Puppet Classes" and "Smart Class Parameters" extrem slow

chr1s · March 2, 2021, 11:45am

To keep it short and simple : Bug #31990: Foreman 2.2.2 - Edit “Puppet Classes” and “Smart Class Parameters” extrem slow - Foreman

tbrisker · March 2, 2021, 4:03pm

Here you go:

github.com/theforeman/foreman

Fixes #31990 - Optimize tooltip and popover initialization

theforeman:develop ← tbrisker:31990

opened 04:00PM - 02 Mar 21 UTC

tbrisker

+16 -22

Instead of iterating over all elements with tooltips or popovers and initializi…ng each one seperately, initialize them once with a selector to identify which elements should have them. On pages with hundreds or thousands of tooltips and popovers this could have a significant performance impact on the page rendering.

would be great if you can report the impact this change has on your environment.

chr1s · March 3, 2021, 7:46am

Sure, I tested again aaannnd the fix seems to work Now it takes 6-8s to load the page (previously without the fix 50+s). Thats a great performance boost. Thank you.

But one more thing, your patch is for the latest code of foreman (i think v2.3.X/2.4.X), right? I tested with 2.2.2 and did not found some of your diff’s in our copy of the source code, e.g. webpack/assets/javascripts/foreman_tools.js -> L137:139

tbrisker · March 3, 2021, 9:14am

The patch is against the current development branch. Looking at the 2.2 branch, this is before highlightTabErrors was moved to webpack, so the only change you need in application.js is changing:

github.com

theforeman/foreman/blob/2.2-stable/app/assets/javascripts/application.js#L55


      
            .first()
            .click();
          $('.nav-pills .tab-error')
            .first()
            .click();
          errorFields
            .first()
            .find('.form-control')
            .focus();
          
          $('a[rel="popover"]').popover();
          tfm.tools.activateTooltips();
          tfm.tools.activateDatatables();
          
          $('a[disabled="disabled"]').click(function(event) {
            return handleDisabledClick(event, this);
          });
          
          // allow opening new window for selected links
          $('a[rel="external"]').attr('target', '_blank');

to:

$('body').popover({selector: 'a[rel="popover"]'});

for a little extra optimization.
The most significant change is inside the activateTooltips function which I assume patched cleanly.

Would be good if you could comment on the PR with the data from your system

chr1s · March 3, 2021, 9:23am

Of course, I can do that (PR == Fixes #31990 - Optimize tooltip and popover initialization by tbrisker · Pull Request #8364 · theforeman/foreman · GitHub, right?). What kind of data do you want to “see”? Request time after the fix, environment size (number of host, puppetclasses, …)?

tbrisker · March 3, 2021, 9:26am

just some of the numbers from this post so it’s easy for the reviewer to understand the impact - number of overrides on the page and time before/after would be great.
Also if you can confirm that the tooltips and popovers still display properly it would be great i tested it on my machine but always best to get another confirmation.

chr1s · March 3, 2021, 10:39am

Done, hopefully it’s enough On my test system tooltips and popovers are still displayed properly (in my eyes).

chr1s · March 4, 2021, 10:05am

@tbrisker , this morning i started to rebuild our foreman package with your fix and deployed it on our integration system.
In this environment we recognized that the edit page of hosts now throw the error “Oops, we’re sorry but something went wrong comparison of Array with Array failed”.
I’m not yet sure, whether this error comes from the fix or from the build dependencies

tbrisker · March 4, 2021, 10:08am

This could not be caused by the fix since it only touches JS code for displaying tooltips, and that error message is coming from the server. there should be a full stacktrace in production.log (though you might need to change your logging level to debug to see it).

chr1s · March 4, 2021, 10:17am

I was just about to collect the data. Here we go foreman_array_with_array.log (24.8 KB)

tbrisker · March 4, 2021, 10:26am

For some reason the classification is failing to properly sort the overrides for the host. is this on every host or just a specific one? could it be related to some change you made to the overrides in one of the previous attempts to speed up the puppet class forms? is the ENC output for the host correct or is it also erroring?

chr1s · March 4, 2021, 11:13am

Sorry, it seems everything to be fine. I couldn’t wait to get the fix in our integration / production environment so i missed to set the right redis cache // config.