Uploaded image for project: '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

      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

        Gliffy Diagrams

        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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            +1 !

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - +1 !
            Hide
            salvetore 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
            salvetore 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 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 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
            tgus Tim Gus added a comment -

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

            Show
            tgus Tim Gus added a comment - I'm currently looking into a solution for this issue.
            Hide
            tgus 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
            tgus 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
            tgus Tim Gus added a comment -

            UI proposal for performance overview page

            Show
            tgus Tim Gus added a comment - UI proposal for performance overview page
            Hide
            quen 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
            quen 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
            rajeshtaneja Rajesh Taneja added a comment -

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

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

            Shifting to the next sprint.

            Show
            salvetore Michael de Raadt added a comment - Shifting to the next sprint.
            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            tsala 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
            tsala 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
            quen 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
            quen 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
            rajeshtaneja 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
            rajeshtaneja 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
            quen 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
            quen 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
            rajeshtaneja 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
            rajeshtaneja 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
            quen 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
            quen 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
            quen 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
            quen 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
            rajeshtaneja 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
            rajeshtaneja 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
            quen 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
            quen 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
            rajeshtaneja 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
            rajeshtaneja 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
            quen 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
            quen 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
            rajeshtaneja 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
            rajeshtaneja 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
            stronk7 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
            stronk7 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
            nebgor 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
            nebgor 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
            rajeshtaneja 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
            rajeshtaneja 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 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 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 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 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
            nebgor 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
            nebgor 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
            tsala 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
            tsala 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 Damyon Wiese added a comment -

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

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

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

            Show
            damyon Damyon Wiese added a comment - Good point about plugins adding themselves here (although they probably wouldn't want to admit it!).
            Hide
            timhunt 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
            timhunt 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
            tsala 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
            tsala 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
            nebgor 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
            nebgor 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            quen 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
            quen 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
            rajeshtaneja 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
            rajeshtaneja 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
            nebgor 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
            nebgor 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
            dmonllao David Monllaó added a comment -

            Working as expected, it passes

            Show
            dmonllao David Monllaó added a comment - Working as expected, it passes
            Hide
            stronk7 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
            stronk7 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
            tsala 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
            tsala Helen Foster added a comment - Please can anyone help writing some documentation (to supplement the report) here: http://docs.moodle.org/en/Performance_overview
            Hide
            marycooch Mary Cooch added a comment -

            Removing the docs_required label as this improvement is now documented: https://docs.moodle.org/en/Performance_overview.

            If you notice that the documentation can be improved, please feel free to log in to the wiki and edit the page. Help in keeping Moodle documentation accurate and up-to-date is much appreciated.

            Show
            marycooch Mary Cooch added a comment - Removing the docs_required label as this improvement is now documented: https://docs.moodle.org/en/Performance_overview . If you notice that the documentation can be improved, please feel free to log in to the wiki and edit the page. Help in keeping Moodle documentation accurate and up-to-date is much appreciated.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13