Moodle
  1. Moodle
  2. MDL-35716

Create performance report page showing warning, which can potentially affect performance.

    Details

    • Testing Instructions:
      Hide
      1. Log in as admin
      2. Go to Site administration ► Reports ► Performance overview
      3. Make sure you can see list of performance issues. (5 issues)
      4. Change values for each config listed on this page and make sure you can see corresponding issue value to be highlighted (red/yellow).
      Show
      Log in as admin Go to Site administration ► Reports ► Performance overview Make sure you can see list of performance issues. (5 issues) Change values for each config listed on this page and make sure you can see corresponding issue value to be highlighted (red/yellow).
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-mdl-35716
    • Rank:
      44460

      Description

      The notifications page shows warnings if your site is not secure. I noticed a forum post recently by somebody who had been complaining about poor performance and where the solution was to turn off theme designer mode...

      In this thread, 3 people said they had experienced this:

      http://moodle.org/mod/forum/discuss.php?d=194581

      To reduce perception of Moodle 2 performance being even worse than the actual performance, I suggest that the notifications page should warn if:

      • theme designer mode is turned on
      • javascript cacheing is turned off
      • debugging is set to 'developer' mode. (Other settings are used in production, here at least, where we want to catch warnings...)

      The basic category should be, settings that are inappropriate for live sites. It could be a box as follows:

      The settings on this site are not appropriate for a live Moodle system:

      • Theme designer mode is turned on. This makes the system much slower. Change
      • Javascript caching is turned off. This makes the system much slower. Change

      (links just go to the relevant settings pages...)

      Not offering to develop this, sorry, just passing it on as an idea

      1. moodle_v2.png
        94 kB
      2. moodle.png
        95 kB
      3. performance_report.png
        117 kB
      4. security_overview.png
        77 kB

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 !

          Show
          Eloy Lafuente (stronk7) added a comment - +1 !
          Hide
          Michael de Raadt added a comment -

          Thanks for the suggestion, Sam.

          It would be nice if we did this in a way that we could add more advice in future. Perhaps that means adding more rows to a table.

          Linking to documentation would need to be a priority.

          Show
          Michael de Raadt added a comment - Thanks for the suggestion, Sam. It would be nice if we did this in a way that we could add more advice in future. Perhaps that means adding more rows to a table. Linking to documentation would need to be a priority.
          Hide
          Jenny Gray added a comment -

          Would you have a way of determining that a site is 'live' and only show the warnings then? Or just show them all the time?

          There are other places where knowing which is your live site with a simple config check could be useful e.g. whether to output google analytics tracking codes in the theme footer or not, disabling sending emails, opentogoogle search indexing ...

          Show
          Jenny Gray added a comment - Would you have a way of determining that a site is 'live' and only show the warnings then? Or just show them all the time? There are other places where knowing which is your live site with a simple config check could be useful e.g. whether to output google analytics tracking codes in the theme footer or not, disabling sending emails, opentogoogle search indexing ...
          Hide
          Tim Gus added a comment -

          I'm currently looking into a solution for this issue.

          Show
          Tim Gus added a comment - I'm currently looking into a solution for this issue.
          Hide
          Tim Gus added a comment -

          I've posted a proposal for a possible solution to this issue: http://docs.moodle.org/24/en/Performance_overview

          I'd like to receive any type of feedback.

          Show
          Tim Gus added a comment - I've posted a proposal for a possible solution to this issue: http://docs.moodle.org/24/en/Performance_overview I'd like to receive any type of feedback.
          Hide
          Tim Gus added a comment -

          UI proposal for performance overview page

          Show
          Tim Gus added a comment - UI proposal for performance overview page
          Hide
          Sam Marshall added a comment -

          I like the proposed interface. I would suggest a few things:

          1) Minor point, but setting debug to any value other than DEVELOPER should not cause a significant performance problem. (I.e. your screenshot is correct to indicate that DEVELOPER may be a performance problem because Moodle's supposed to do extra checks in that case; but Moodle should perform well even if set to ALL which is the value one below developer; you shouldn't have to set it to NONE.) At the OU we run our live systems on ALL (or some such level, but anyway, not NONE) because otherwise you don't get php logging of important failures.

          2) Re the last column 'Suggestion':
          a) The wording could be tightened - 'full sentences' are not good when repeated frequently, so it's better to come up with a way to reduce the duplication of text.
          b) We know slightly more about performance than the wording in that screenshot suggests.

          One possibility is to add two columns instead of the one you've got at the right, so that the information is like this:

          Suggestion: Disable option
          Impact: Significant

          or

          Suggestion: Enable option
          Impact: Possible

          or

          Suggestion: Set to any value other than DEVELOPER
          Impact: Significant

          From your example, theme designer mode and JS cache will definitely affect performance (on any system where there are any performance limitations), statistics and backup may affect performance but this depends on the system. I'm not sure 'significant' and 'possible' are the right words, but there could also be a key at the bottom explaining what they mean. Obviously there might be other words, like 'minor' as well, or you could use different words.

          Any 'significant' options with the wrong setting could appear in bold.

          3) I think this page should contain text (at the top?) indicating that it shows common problems with moodle options that can cause performance problem, and more information about performance is available on ... (link to relevant moodledocs page, which might just be the same as the 'help with this page' link). In other words although this page shows some performance problems it is not going to be the last word and I think that should be indicated up front.

          Show
          Sam Marshall added a comment - I like the proposed interface. I would suggest a few things: 1) Minor point, but setting debug to any value other than DEVELOPER should not cause a significant performance problem. (I.e. your screenshot is correct to indicate that DEVELOPER may be a performance problem because Moodle's supposed to do extra checks in that case; but Moodle should perform well even if set to ALL which is the value one below developer; you shouldn't have to set it to NONE.) At the OU we run our live systems on ALL (or some such level, but anyway, not NONE) because otherwise you don't get php logging of important failures. 2) Re the last column 'Suggestion': a) The wording could be tightened - 'full sentences' are not good when repeated frequently, so it's better to come up with a way to reduce the duplication of text. b) We know slightly more about performance than the wording in that screenshot suggests. One possibility is to add two columns instead of the one you've got at the right, so that the information is like this: Suggestion: Disable option Impact: Significant or Suggestion: Enable option Impact: Possible or Suggestion: Set to any value other than DEVELOPER Impact: Significant From your example, theme designer mode and JS cache will definitely affect performance (on any system where there are any performance limitations), statistics and backup may affect performance but this depends on the system. I'm not sure 'significant' and 'possible' are the right words, but there could also be a key at the bottom explaining what they mean. Obviously there might be other words, like 'minor' as well, or you could use different words. Any 'significant' options with the wrong setting could appear in bold. 3) I think this page should contain text (at the top?) indicating that it shows common problems with moodle options that can cause performance problem, and more information about performance is available on ... (link to relevant moodledocs page, which might just be the same as the 'help with this page' link). In other words although this page shows some performance problems it is not going to be the last word and I think that should be indicated up front.
          Hide
          Rajesh Taneja added a comment -

          Adding this to next sprint to have a look at it.

          Show
          Rajesh Taneja added a comment - Adding this to next sprint to have a look at it.
          Hide
          Michael de Raadt added a comment -

          Shifting to the next sprint.

          Show
          Michael de Raadt added a comment - Shifting to the next sprint.
          Hide
          Rajesh Taneja added a comment -

          I have created performance report page which is designed in same way as security report (to keep similar behaviour).

          Added Helen to review lang strings.

          Not sure if we can add any other config settings.
          Also, need feeback on doc links, I think we should create doc page to specify these problems as done for security report.

          In addition to this report, it will be nice to display critical/serious warning on notifications page. Probably open another improvement after finalising this.

          Show
          Rajesh Taneja added a comment - I have created performance report page which is designed in same way as security report (to keep similar behaviour). Added Helen to review lang strings. Not sure if we can add any other config settings. Also, need feeback on doc links, I think we should create doc page to specify these problems as done for security report. In addition to this report, it will be nice to display critical/serious warning on notifications page. Probably open another improvement after finalising this.
          Hide
          Helen Foster added a comment -

          Raj, thanks for adding me as a watcher. https://tracker.moodle.org/secure/attachment/31443/performance_report.png looks great! Here are my suggestions for rewording:

          1. Rename the first column 'Issue' (then 'Backup: Active' can be renamed 'Automated backup' and 'Enable statistics' can be renamed 'Statistics')
          2. Use 'Enabled' rather than 'Enable'
          3. Rename the third column 'Comments' or 'Remarks'

          The comments could then be reworded as follows:

          Theme designer mode - If disabled, images and style sheets are cached, resulting in significant performance improvements.
          Cache Javascript - If enabled, page loading performance is improved.
          Debug messages - If set to DEVELOPER, performance may be affected slightly.
          Automated backup - Performance may be affected during the backup process. If enabled, backups should be scheduled for off-peak times.
          Statistics - Performance may be affected by statistics processing. If enabled, statistics settings should be set with caution.

          Alternatively, if we follow Sam's suggestion 2), we could have a third column named 'Recommended setting' and a fourth column 'Impact on performance'

          Theme designer mode - Disabled - Significant
          Cache Javascript - Enabled - Significant
          Debug messages - NONE - Minor
          Automated backup - Disabled - Possible
          Statistics - Disabled - Possible

          I think I prefer the longer comments, since it explains that for backup and stats, performance is only affected when the processes are running - they don't have to be disabled completely.

          Regarding Sam's suggestion 3) how about:

          This report lists issues which may affect performance of the site. 'More help'

          where is the help icon and 'More help' links to report/performance in the docs (as does the 'Moodle Docs for this page' link in the page footer).

          PS I'm attaching a screenshot of the security overview report for comparison.

          Show
          Helen Foster added a comment - Raj, thanks for adding me as a watcher. https://tracker.moodle.org/secure/attachment/31443/performance_report.png looks great! Here are my suggestions for rewording: Rename the first column 'Issue' (then 'Backup: Active' can be renamed 'Automated backup' and 'Enable statistics' can be renamed 'Statistics') Use 'Enabled' rather than 'Enable' Rename the third column 'Comments' or 'Remarks' The comments could then be reworded as follows: Theme designer mode - If disabled, images and style sheets are cached, resulting in significant performance improvements. Cache Javascript - If enabled, page loading performance is improved. Debug messages - If set to DEVELOPER, performance may be affected slightly. Automated backup - Performance may be affected during the backup process. If enabled, backups should be scheduled for off-peak times. Statistics - Performance may be affected by statistics processing. If enabled, statistics settings should be set with caution. Alternatively, if we follow Sam's suggestion 2), we could have a third column named 'Recommended setting' and a fourth column 'Impact on performance' Theme designer mode - Disabled - Significant Cache Javascript - Enabled - Significant Debug messages - NONE - Minor Automated backup - Disabled - Possible Statistics - Disabled - Possible I think I prefer the longer comments, since it explains that for backup and stats, performance is only affected when the processes are running - they don't have to be disabled completely. Regarding Sam's suggestion 3) how about: This report lists issues which may affect performance of the site. 'More help' where is the help icon and 'More help' links to report/performance in the docs (as does the 'Moodle Docs for this page' link in the page footer). PS I'm attaching a screenshot of the security overview report for comparison.
          Hide
          Sam Marshall added a comment -

          Note: I quickly skimmed the code - apart from some whitespace errors and possible minor code duplication it looked OK. Would you like me to do a proper code review or is somebody else going to do that?

          Show
          Sam Marshall added a comment - Note: I quickly skimmed the code - apart from some whitespace errors and possible minor code duplication it looked OK. Would you like me to do a proper code review or is somebody else going to do that?
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Helen and Sam.

          I have integrated changes suggested by Helen.

          @Sam: It will be nice if you can review this for me. I ran my code though code checker and it didn't report about whitespace. Anyway looking forward for more feedback to get this ready for integration.

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Helen and Sam. I have integrated changes suggested by Helen. @Sam: It will be nice if you can review this for me. I ran my code though code checker and it didn't report about whitespace. Anyway looking forward for more feedback to get this ready for integration.
          Hide
          Sam Marshall added a comment -

          Detailed review comments, all IMO etc:

          1. Top comment in report/performance/index.php still says 'security'

          2. Incorrect whitespace around lines:

          $strissue    = get_string('issue', 'report_performance')
          

          There should only be one space before/after the = in these lines.

          3. Probably wrong indent:

          + $statusarr = array(REPORT_PERFORMANCE_OK       => 'statusok',
          +                   REPORT_PERFORMANCE_WARNING  => 'statuswarning',
          

          Should probably be 8 spaces (or 4 spaces if you want to use the 'array' style but in that case I think you want an LF before the first one).

          4. Missing space around => (codechecker doesn't spot this...)

          +$table->attributes = array('class'=>'admintable performancereport generaltable');
          

          5. Code duplication regarding single issue / whole list

          It seems like several lines of code about displaying the issue is duplicated between the single issue / whole table case. Could duplication be reduced by using a local function (defined in this file I guess) to return the $row, with parameter for the first part which is different?

          6. Lang file & locallib file has no newline at end. (Codechecker should spot this, really! Oh well.)

          7. Minor phpdoc issues

          a) In report_performance_doc_link function & maybe some others there isn't a blank line between the description and the @params (I think there should be, most of the other functions have it)

          b) In same function the @return is not defined, e.g. it could be @return string HTML code for document link - some kind of text not blank, imo...

          c) Some place uses @return stdClass, I think in phpdoc it is generally used @return object (instead of stdClass), but see below

          8. Those stdclass returns

          Rather than using stdclass for those returns, since it is a clearly defined structure used in multiple places, it would be better to define a class at the top of this file like

          class report_performance_result

          Which defines the fields (can be public if you like so wouldn't require code changes except for the 'new' line and phpdoc object type).

          Show
          Sam Marshall added a comment - Detailed review comments, all IMO etc: 1. Top comment in report/performance/index.php still says 'security' 2. Incorrect whitespace around lines: $strissue = get_string('issue', 'report_performance') There should only be one space before/after the = in these lines. 3. Probably wrong indent: + $statusarr = array(REPORT_PERFORMANCE_OK => 'statusok', + REPORT_PERFORMANCE_WARNING => 'statuswarning', Should probably be 8 spaces (or 4 spaces if you want to use the 'array' style but in that case I think you want an LF before the first one). 4. Missing space around => (codechecker doesn't spot this...) +$table->attributes = array('class'=>'admintable performancereport generaltable'); 5. Code duplication regarding single issue / whole list It seems like several lines of code about displaying the issue is duplicated between the single issue / whole table case. Could duplication be reduced by using a local function (defined in this file I guess) to return the $row, with parameter for the first part which is different? 6. Lang file & locallib file has no newline at end. (Codechecker should spot this, really! Oh well.) 7. Minor phpdoc issues a) In report_performance_doc_link function & maybe some others there isn't a blank line between the description and the @params (I think there should be, most of the other functions have it) b) In same function the @return is not defined, e.g. it could be @return string HTML code for document link - some kind of text not blank, imo... c) Some place uses @return stdClass, I think in phpdoc it is generally used @return object (instead of stdClass), but see below 8. Those stdclass returns Rather than using stdclass for those returns, since it is a clearly defined structure used in multiple places, it would be better to define a class at the top of this file like class report_performance_result Which defines the fields (can be public if you like so wouldn't require code changes except for the 'new' line and phpdoc object type).
          Hide
          Rajesh Taneja added a comment -

          Thanks for the through review Sam,

          I have integrated changes as suggested by you. As per using object instead of stdClass, it should be stdClass as per coding style guide.

          FYI: Last commit refractors code to use classes, in start I just followed same procedural design as used in security report, but Sam's last comment made me think of using classes. I will merge commits after peer-review.

          Show
          Rajesh Taneja added a comment - Thanks for the through review Sam, I have integrated changes as suggested by you. As per using object instead of stdClass, it should be stdClass as per coding style guide . FYI: Last commit refractors code to use classes, in start I just followed same procedural design as used in security report, but Sam's last comment made me think of using classes. I will merge commits after peer-review.
          Hide
          Sam Marshall added a comment -

          You are right about stdClass - for some reason I had the impression that although stdClass is correct in code, object was correct in phpdoc, but it looks like I'm wrong. Sorry about that.

          Show
          Sam Marshall added a comment - You are right about stdClass - for some reason I had the impression that although stdClass is correct in code, object was correct in phpdoc, but it looks like I'm wrong. Sorry about that.
          Hide
          Sam Marshall added a comment -

          About the class reorganisation - sorry but I think the way this has been done is a bit hard to understand This might just be my opinion. But basically, there is a single object of the issues class, which then gets sort of 'reset' for each issue by calling the function. That just seems weird. Also I don't really like the way it uses reflection (list of functions) to get the issue list, that means if somebody decided to add some other utility function to that issues class it would think it's a test again...

          I would suggest:

          • Move the specific issue test functions into being static functions in report_performance, which each return a new report_performance_issue object
          • Put the list of tests back (or you could do it by reflection if you use a specific name, like all the static functions that start with check_ or something).
          • report_performance_issue then becomes a really small class with no functions (except maybe a constructor if you like) just to represent the data associated with one particular issue.

          Does this make sense? You might want to get a second opinion.

          Show
          Sam Marshall added a comment - About the class reorganisation - sorry but I think the way this has been done is a bit hard to understand This might just be my opinion. But basically, there is a single object of the issues class, which then gets sort of 'reset' for each issue by calling the function. That just seems weird. Also I don't really like the way it uses reflection (list of functions) to get the issue list, that means if somebody decided to add some other utility function to that issues class it would think it's a test again... I would suggest: Move the specific issue test functions into being static functions in report_performance, which each return a new report_performance_issue object Put the list of tests back (or you could do it by reflection if you use a specific name, like all the static functions that start with check_ or something). report_performance_issue then becomes a really small class with no functions (except maybe a constructor if you like) just to represent the data associated with one particular issue. Does this make sense? You might want to get a second opinion.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the review Sam,

          Defining functions as static defeat purpose of oops, so I am not sure if that is a good idea.

          To make it more readable, I have added abstract class. If you want I can separate out all the issues in different classes (but that might be overdoing it.)

          Please let me know your thoughts on this.

          Show
          Rajesh Taneja added a comment - Thanks for the review Sam, Defining functions as static defeat purpose of oops, so I am not sure if that is a good idea. To make it more readable, I have added abstract class. If you want I can separate out all the issues in different classes (but that might be overdoing it.) Please let me know your thoughts on this.
          Hide
          Sam Marshall added a comment -

          Hi Rajesh. In this case I don't think it really 'defeats the purpose of oops' to make the functions static - the way your non-static functions were defined already didn't make sense with regard to objects. And they're basically like static functions, except not static, which is weird.

          The basic point is that you had an 'issues' object which you were then going to store information about one issue at a time setting up each one from a different function - this makes no sense from an object perspective. From an object perspective what you should have is an 'issue' object, and then you create more than one of them (in an array) to represent each issue as needed. You don't need functions to do stuff to them, only a constructor to set each one up.

          As you suggest, if you want to do it 'properly' OOP (not a massive benefit but there's no reason not to do it either), you would need to make a separate class for each issue. If you do that, the base class is fine, just turn the other functions into constructors. So you can make your get_issue function do basically,

          return new report_performance_issue_cachejs();

          where report_performance_issue_cachejs is a new class that extends your abstract class and has a constructor which is the same as the existing cachejs function.

          (obviously you'd have to make the classname out of a string variable rather than literally writing it out like that)

          Feel free to pass this to somebody else if you don't think I'm making sense here, or you just disagree.

          Show
          Sam Marshall added a comment - Hi Rajesh. In this case I don't think it really 'defeats the purpose of oops' to make the functions static - the way your non-static functions were defined already didn't make sense with regard to objects. And they're basically like static functions, except not static, which is weird. The basic point is that you had an 'issues' object which you were then going to store information about one issue at a time setting up each one from a different function - this makes no sense from an object perspective. From an object perspective what you should have is an 'issue' object, and then you create more than one of them (in an array) to represent each issue as needed. You don't need functions to do stuff to them, only a constructor to set each one up. As you suggest, if you want to do it 'properly' OOP (not a massive benefit but there's no reason not to do it either), you would need to make a separate class for each issue. If you do that, the base class is fine, just turn the other functions into constructors. So you can make your get_issue function do basically, return new report_performance_issue_cachejs(); where report_performance_issue_cachejs is a new class that extends your abstract class and has a constructor which is the same as the existing cachejs function. (obviously you'd have to make the classname out of a string variable rather than literally writing it out like that) Feel free to pass this to somebody else if you don't think I'm making sense here, or you just disagree.
          Hide
          Rajesh Taneja added a comment -

          Thanks for bearing with me Sam,

          I have added hard-coded list of issues and moved all functions as static to report_performance (As suggested earlier).

          Hope this is fine now.

          Show
          Rajesh Taneja added a comment - Thanks for bearing with me Sam, I have added hard-coded list of issues and moved all functions as static to report_performance (As suggested earlier). Hope this is fine now.
          Hide
          Sam Marshall added a comment -

          Perfect - sorry for being a pain +1 from me to submit. (I only looked at those changed details this time.)

          Show
          Sam Marshall added a comment - Perfect - sorry for being a pain +1 from me to submit. (I only looked at those changed details this time.)
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,

          I am happy to get your review on this.
          It's good to get it right the first time, rather then fixing it over many iterations.

          Pushing it for integration review.

          Show
          Rajesh Taneja added a comment - Thanks Sam, I am happy to get your review on this. It's good to get it right the first time, rather then fixing it over many iterations. Pushing it for integration review.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Aparup Banerjee added a comment -

          Hi Raj,
          hope you've had a good weekend , here's a few quick notes -

          • its currently showing up as 'Add-on' in plugins overview. since we're integrating this into core, please whitelist this in lib/pluginlib.php::standard_plugins_list() near 'report' => array(..)
          • the value for 'debugging' doesn't match with the search link under 'edit' column : the search results show many more. ah apparently it isn't easy to just show the 'debug' setting alone in the search results - perhaps this could be considered as a separate improvement (usability for admins lol).
          • can we consider providing a link on the setting itself that points to the 'Performance overview page->Issue' page? this way someone playing around with settings can also be alerted that this one setting will have performance issues. Perhaps another issue could be some beautiful finger icon warning about performance for the setting, index finger . What do you think?
          • also following from the above : please update the navbar to show the single issue page ie: "Home / ▶ Site administration / ▶ Reports / ▶ Performance overview " + "▶ Enable Statistics"
          • if we can denote that a particular setting does affect performance, then perhaps we don't need a hardcoding in get_issue_list() and this can be retrieved via a one DB call. This way Add-ons can also denote that their settings might affect performance and it will show up in this screen(s).

          So really i think what we're trying to do here is create a report on settings that affect performance AND extend our settings system to portray what settings affect performance. I don't think its a good idea to mix up the report with the settings system we have. REPORT_PERFORMANCE_OK and ..WARNING can be centrally defined at the individual setting level and more readily accessible to anywhere else too.

          surely if we warn at the place a setting is set - is better than - warn in a page that you've to go to , to check. The config table could have some mask in a performance column.

          As a reference, think of the capabilities assignment screens with its triangle icons.

          shall we rethink this a little?

          Show
          Aparup Banerjee added a comment - Hi Raj, hope you've had a good weekend , here's a few quick notes - its currently showing up as 'Add-on' in plugins overview. since we're integrating this into core, please whitelist this in lib/pluginlib.php::standard_plugins_list() near 'report' => array(..) the value for 'debugging' doesn't match with the search link under 'edit' column : the search results show many more. ah apparently it isn't easy to just show the 'debug' setting alone in the search results - perhaps this could be considered as a separate improvement (usability for admins lol). can we consider providing a link on the setting itself that points to the 'Performance overview page->Issue' page? this way someone playing around with settings can also be alerted that this one setting will have performance issues. Perhaps another issue could be some beautiful finger icon warning about performance for the setting, index finger . What do you think? also following from the above : please update the navbar to show the single issue page ie: "Home / ▶ Site administration / ▶ Reports / ▶ Performance overview " + "▶ Enable Statistics" if we can denote that a particular setting does affect performance, then perhaps we don't need a hardcoding in get_issue_list() and this can be retrieved via a one DB call. This way Add-ons can also denote that their settings might affect performance and it will show up in this screen(s). So really i think what we're trying to do here is create a report on settings that affect performance AND extend our settings system to portray what settings affect performance. I don't think its a good idea to mix up the report with the settings system we have. REPORT_PERFORMANCE_OK and ..WARNING can be centrally defined at the individual setting level and more readily accessible to anywhere else too. surely if we warn at the place a setting is set - is better than - warn in a page that you've to go to , to check. The config table could have some mask in a performance column. As a reference, think of the capabilities assignment screens with its triangle icons. shall we rethink this a little?
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Aparup,

          1. I have added performance to core plugin list.
          2. With current search functionality, this is not possible. As you mentioned, it's a different issue if need be.
          3. I am not sure if that is required. As we have description next to setting, user knows what he/she is doing. Although link issue should show any problems on notification page (Which is ideal)
          4. This has been kept similar to performance overview report. If this list grows, it will be tough manage through navigation list (Let me know your thoughts and I will change this.)
          5. With current implementation of config settings, we can't get this list. We have to hard-code it. For Add-on's we can probably provide a callback function, but then it should be done for both security and performance.
          Show
          Rajesh Taneja added a comment - Thanks for the feedback Aparup, I have added performance to core plugin list. With current search functionality, this is not possible. As you mentioned, it's a different issue if need be. I am not sure if that is required. As we have description next to setting, user knows what he/she is doing. Although link issue should show any problems on notification page (Which is ideal) This has been kept similar to performance overview report. If this list grows, it will be tough manage through navigation list (Let me know your thoughts and I will change this.) With current implementation of config settings, we can't get this list. We have to hard-code it. For Add-on's we can probably provide a callback function, but then it should be done for both security and performance.
          Hide
          Damyon Wiese added a comment -

          I like the report - it would be good to get someone to check all the new strings - there are some that could use some improvement:

          "If set to DEVELOPER, performance may be affected slightly."

          Affected how? - is that good or bad?

          Also - Minor typo - never heard of jacascript.

          Also - Would prefer we had a standard warning class we could use in all admin pages instead of listing them individually.

          Show
          Damyon Wiese added a comment - I like the report - it would be good to get someone to check all the new strings - there are some that could use some improvement: "If set to DEVELOPER, performance may be affected slightly." Affected how? - is that good or bad? Also - Minor typo - never heard of jacascript. Also - Would prefer we had a standard warning class we could use in all admin pages instead of listing them individually.
          Hide
          Damyon Wiese added a comment -

          Re: Adding a flag to the admin_setting class - I would prefer we didn't. I think there will be a limited number of these settings - and dealing with an extra flag on every setting will have a (slight) performance impact. I think keeping this contained within this report works nicely (you need to add the strings to describe the performance impact anyway).

          Show
          Damyon Wiese added a comment - Re: Adding a flag to the admin_setting class - I would prefer we didn't. I think there will be a limited number of these settings - and dealing with an extra flag on every setting will have a (slight) performance impact. I think keeping this contained within this report works nicely (you need to add the strings to describe the performance impact anyway).
          Hide
          Aparup Banerjee added a comment -

          The adding a column - yes agreed its too rare a case to have an entire column.
          Raj had mentioned using callback mechanism to have this defined instead.
          The main thing i see here is that plugins need to be able to define this as well somehow in their settings, otherwise a moodle site could still be under performing and nothing would show up regarding performance on this or the (linked issue) notifications page. Atm, these notifications are core only and not modular.

          Show
          Aparup Banerjee added a comment - The adding a column - yes agreed its too rare a case to have an entire column. Raj had mentioned using callback mechanism to have this defined instead. The main thing i see here is that plugins need to be able to define this as well somehow in their settings, otherwise a moodle site could still be under performing and nothing would show up regarding performance on this or the (linked issue) notifications page. Atm, these notifications are core only and not modular.
          Hide
          Helen Foster added a comment -

          "If set to DEVELOPER, performance may be affected slightly."

          Affected how? - is that good or bad?

          If someone can explain how performance might be affected by turning debugging on, I'd be happy to rewrite the string. (I wasn't aware that debugging could possibly affect performance.)

          Show
          Helen Foster added a comment - "If set to DEVELOPER, performance may be affected slightly." Affected how? - is that good or bad? If someone can explain how performance might be affected by turning debugging on, I'd be happy to rewrite the string. (I wasn't aware that debugging could possibly affect performance.)
          Hide
          Damyon Wiese added a comment -

          Thanks Helen - turning debugging on will make Moodle run slower.

          Show
          Damyon Wiese added a comment - Thanks Helen - turning debugging on will make Moodle run slower.
          Hide
          Damyon Wiese added a comment -

          Good point about plugins adding themselves here (although they probably wouldn't want to admit it!).

          Show
          Damyon Wiese added a comment - Good point about plugins adding themselves here (although they probably wouldn't want to admit it!).
          Hide
          Tim Hunt added a comment -

          To answer the immediate question, turning on debugging will make Moodle run very fractionally slower, but it is really not worth worrying about.

          More generally, I am confused about that is happening in this issue. It started off as a simple suggestion to add a warning directly on the admin notifications page about a few settings, like 'Theme designer mode' that totally destroy performance on live sites, and which we know from reading the forums people sometimes get wrong.

          That was a great idea that should have been simple to do.

          Now, we seem to be heading in the direction of something over-engineered. Why are there phrases like "Adding a flag to the admin_setting class" or "using callback mechanism" in the comments above?

          Can't we just get a simple solution integrated?

          Show
          Tim Hunt added a comment - To answer the immediate question, turning on debugging will make Moodle run very fractionally slower, but it is really not worth worrying about. More generally, I am confused about that is happening in this issue. It started off as a simple suggestion to add a warning directly on the admin notifications page about a few settings, like 'Theme designer mode' that totally destroy performance on live sites, and which we know from reading the forums people sometimes get wrong. That was a great idea that should have been simple to do. Now, we seem to be heading in the direction of something over-engineered. Why are there phrases like "Adding a flag to the admin_setting class" or "using callback mechanism" in the comments above? Can't we just get a simple solution integrated?
          Hide
          Helen Foster added a comment -

          +1 for a simple solution.

          If turning on debugging only makes Moodle run very fractionally slower, is it worth mentioning it at all? The docs recommend anyway to turn debugging off once a problem is diagnosed.

          Show
          Helen Foster added a comment - +1 for a simple solution. If turning on debugging only makes Moodle run very fractionally slower, is it worth mentioning it at all? The docs recommend anyway to turn debugging off once a problem is diagnosed.
          Hide
          Aparup Banerjee added a comment -

          Tim, this was reopened due to some fixes needed, Raj has since then pushed some fixes in and i'm sure its improved now so that it can be integrated. (Any improvements are generally integrated)

          In addition to that i've aired my concerns about the hard coding of the 'list of performance issues' in the code and how it doesn't cater to modular design - i'm sure there can be a light weight (not so over-engineered) solution to allowing performance related settings to be shared. The fact remains, plugins should be able somehow to share any setting it has that can affect performance. Performance issues aren't exclusive to core alone.

          This was a discussion about a great idea. Not a NO to a great idea

          Show
          Aparup Banerjee added a comment - Tim, this was reopened due to some fixes needed, Raj has since then pushed some fixes in and i'm sure its improved now so that it can be integrated. (Any improvements are generally integrated) In addition to that i've aired my concerns about the hard coding of the 'list of performance issues' in the code and how it doesn't cater to modular design - i'm sure there can be a light weight (not so over-engineered) solution to allowing performance related settings to be shared. The fact remains, plugins should be able somehow to share any setting it has that can affect performance. Performance issues aren't exclusive to core alone. This was a discussion about a great idea. Not a NO to a great idea
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Sam Marshall added a comment -

          IMO, I think warning about debug at DEVELOPER level is correct. Every other level of debugging should not affect performance at all (except that there might be a few extra lines of debugging code output) but, on developer level, code is allowed to do extra tests to make sure things are valid, or even to disable caching on something to make it easier while developing, etc.

          So in other words, even if developer mode doesn't make it much slower right this instant, I think it's supposed to be possible for people to add things to developer mode that will make it a bit slower.

          Also IMO, I think the location of these checks as a separate report is fine (can tell people to check the report as first thing on the list if they have performance concerns) and it's not hugely complicated code. Adding extra reports from plugins could be done later within this infrastructure I think...

          Show
          Sam Marshall added a comment - IMO, I think warning about debug at DEVELOPER level is correct. Every other level of debugging should not affect performance at all (except that there might be a few extra lines of debugging code output) but, on developer level, code is allowed to do extra tests to make sure things are valid, or even to disable caching on something to make it easier while developing, etc. So in other words, even if developer mode doesn't make it much slower right this instant, I think it's supposed to be possible for people to add things to developer mode that will make it a bit slower. Also IMO, I think the location of these checks as a separate report is fine (can tell people to check the report as first thing on the list if they have performance concerns) and it's not hugely complicated code. Adding extra reports from plugins could be done later within this infrastructure I think...
          Hide
          Rajesh Taneja added a comment -

          Thanks everyone,

          pushing it back for integration.
          Changes done since then:

          1. Fixed typo in comment
          2. Added performance to core plugin list.
          Show
          Rajesh Taneja added a comment - Thanks everyone, pushing it back for integration. Changes done since then: Fixed typo in comment Added performance to core plugin list.
          Hide
          Aparup Banerjee added a comment - - edited

          This has been integrated into master now.

          Thanks for the fixes and pointing out the referential design in the 'Security overview' report Raj.

          Edit: created MDL-38432 to look at security and performance report possibly including plugin info.

          Show
          Aparup Banerjee added a comment - - edited This has been integrated into master now. Thanks for the fixes and pointing out the referential design in the 'Security overview' report Raj. Edit: created MDL-38432 to look at security and performance report possibly including plugin info.
          Hide
          David Monllaó added a comment -

          Working as expected, it passes

          Show
          David Monllaó added a comment - Working as expected, it passes
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

          Show
          Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks! PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!
          Hide
          Helen Foster added a comment -

          Please can anyone help writing some documentation (to supplement the report) here: http://docs.moodle.org/en/Performance_overview

          Show
          Helen Foster added a comment - Please can anyone help writing some documentation (to supplement the report) here: http://docs.moodle.org/en/Performance_overview

            People

            • Votes:
              13 Vote for this issue
              Watchers:
              15 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: