Moodle
  1. Moodle
  2. MDL-41266

Define logging interfaces in core

    Details

    • Testing Instructions:
      Hide

      1/ execute phpunit tests - this should prove the legacy logging (log table) still works, new APIs should be covered by tests
      2/ it is hard to test the APIs manually because they will be used by altered reports that use the new log readers - see blocked issues
      3/ go through all subtasks and test according to instructions there if present

      Show
      1/ execute phpunit tests - this should prove the legacy logging (log table) still works, new APIs should be covered by tests 2/ it is hard to test the APIs manually because they will be used by altered reports that use the new log readers - see blocked issues 3/ go through all subtasks and test according to instructions there if present
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Epic Link:
    • Pull from Repository:
    • Pull Master Branch:
      w10_MDL-41266_m27_logging
    • Story Points:
      8
    • Rank:
      40
    • Sprint:
      BACKEND Sprint 11

      Description

      • Define logging interfaces in core
      • Define the expected log table structure with extra request information

      Interfaces are listed in : http://docs.moodle.org/dev/Logging_2#Public_API

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Are you going to post about this in the forums? Are you going to write a summary on Moodle docs?

          Show
          Tim Hunt added a comment - Are you going to post about this in the forums? Are you going to write a summary on Moodle docs?
          Hide
          Petr Škoda added a comment -

          Hello Tim, yes, we are trying to come up with final spec for the API this week and we will post about it in the forums.

          Show
          Petr Škoda added a comment - Hello Tim, yes, we are trying to come up with final spec for the API this week and we will post about it in the forums.
          Hide
          Rajesh Taneja added a comment -

          Thanks for working on this Petr,

          Patch looks promising, few points to consider:

          1. Can we have store implements \tool_log\log\writer, \core\log\sql_reader only and not store implements \tool_log\log\store, \tool_log\log\writer, \core\log\sql_reader
          2. It would be nice to have manager::store renamed to manager::process or something as event is processed by manager and not stored.
          3. Reader is currently accessible by admin only, should we consider new capability for this? As teacher don't have to be admin to see completion/course activity report etc.
          4. Should store be implementing data archiving? If yes, then should it be a part of interface? as that can be used by cron.
          5. It might be nice to reorder fields in logstore_standard/install.xml ...probably grouping userid, realuser and relateduserid together and moving timecreated in last and courseid before contextid.
          6. Lastly design seems asymmetric with placement of writer and reader but as discussed it make sense. For design reasons it just looks bit different.

          Will play with this more and will provide feedback.

          Show
          Rajesh Taneja added a comment - Thanks for working on this Petr, Patch looks promising, few points to consider: Can we have store implements \tool_log\log\writer, \core\log\sql_reader only and not store implements \tool_log\log\store, \tool_log\log\writer, \core\log\sql_reader It would be nice to have manager::store renamed to manager::process or something as event is processed by manager and not stored. Reader is currently accessible by admin only, should we consider new capability for this? As teacher don't have to be admin to see completion/course activity report etc. Should store be implementing data archiving? If yes, then should it be a part of interface? as that can be used by cron. It might be nice to reorder fields in logstore_standard/install.xml ...probably grouping userid, realuser and relateduserid together and moving timecreated in last and courseid before contextid. Lastly design seems asymmetric with placement of writer and reader but as discussed it make sense. For design reasons it just looks bit different. Will play with this more and will provide feedback.
          Hide
          Petr Škoda added a comment -

          The linked specification page should be mostly up-to-date, I am going to improve it a bit tomorrow.

          The linked patch demonstrated the use of APIs proposed in the specification.

          Submitting for first peer review and public discussion, I am going to post oon moodle.org forums tomorrow.

          Thanks everybody for working on this!

          Show
          Petr Škoda added a comment - The linked specification page should be mostly up-to-date, I am going to improve it a bit tomorrow. The linked patch demonstrated the use of APIs proposed in the specification. Submitting for first peer review and public discussion, I am going to post oon moodle.org forums tomorrow. Thanks everybody for working on this!
          Hide
          Matteo Scaramuccia added a comment -

          Hi Petr,
          what an amazing work! I didn't have the chance to read about it 'till few minutes ago thanks to your community post. First newbie questions:

          1. actlogshhdr: what does hhdr mean here? Typo in doubling the h?
          2. edulevel: given the level reserved word issue, what does edu means here? Simply educational as described in http://docs.moodle.org/dev/Event_2#Level_property, the level of educational value of the event?

          Sorry for the low level of the questions, they are just the result of a really first read of the code, need time to understand the overall architecture (need of reading the docs first!) to provide more interesting and constructive feedbacks .
          Note: nice to see that the Event verb list has considered Tin Can as part of the literature too... love to see, one day, a reader able to export the logs into Tin Can Statements.

          Show
          Matteo Scaramuccia added a comment - Hi Petr, what an amazing work! I didn't have the chance to read about it 'till few minutes ago thanks to your community post. First newbie questions: actlogshhdr : what does hhdr mean here? Typo in doubling the h ? edulevel : given the level reserved word issue , what does edu means here? Simply educational as described in http://docs.moodle.org/dev/Event_2#Level_property , the level of educational value of the event ? Sorry for the low level of the questions, they are just the result of a really first read of the code, need time to understand the overall architecture (need of reading the docs first!) to provide more interesting and constructive feedbacks . Note: nice to see that the Event verb list has considered Tin Can as part of the literature too... love to see, one day, a reader able to export the logs into Tin Can Statements .
          Hide
          Petr Škoda added a comment -

          Hi Matteo, 1/ yes a typo, 2/ yep, the original idea behind the level was to separate the learning/teaching/course management event. I guess the edulevel is still open for discussion, but we need something that is not a reserved work in SQL.

          Show
          Petr Škoda added a comment - Hi Matteo, 1/ yes a typo, 2/ yep, the original idea behind the level was to separate the learning/teaching/course management event. I guess the edulevel is still open for discussion, but we need something that is not a reserved work in SQL.
          Hide
          Michael de Raadt added a comment -

          Carrying over to next sprint.

          Show
          Michael de Raadt added a comment - Carrying over to next sprint.
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Petr,
          Nice work so far. I had another look at the code so far. A few things I noticed :-

          1. It would be nice to have tool_log\log\manager::get_writers() , manager::get_enabled_stores(), manager::get_enabled_readers() and manager::get_enabled_writers() , this will also simplify tool_log_setting_managestores::output_html().
          2. core_component is not used in admin/tool/log/classes/plugininfo/logstore.php , but included (or is it included for settings.php?).
          3. APIs in admin/tool/log/classes/plugininfo/logstore.php needs phpdocs.
          4. What are possible uses of tool_log_setting_managestores::is_related? Won't it create problems when we have plugins with similar names like "external" and "legacyexternal"?
          5. tool_log_setting_managestores::output_html has unused vars $CFG and $DB.
          6. logstore_database\log\store has apis with missing phpdocs
          7. Global namespace specifier is not required for function (\get_config) in logstore_database\log\store::get_config()
          8. Following code will be duplicated by most stores. We need to find a way to do this at a higher level:
                    $data = $event->get_data();
                    $data['other'] = serialize($data['other']);
                    if (CLI_SCRIPT) {
                        $data['origin'] = 'cli';
                    } else {
                        $data['origin'] = getremoteaddr();
                    }
                    $data['realuserid'] = \core\session\manager::is_loggedinas() ? $_SESSION['USER']->realuser : null;
            
          9. Also a lot of methods would have almost identical code for many stores, for example get_name(), get_description(), get_config() etc. May be we should consider using an abstract class or trait here?
          10. Also in the same file Behat\Mink\Exception\Exception is included but not used.
          11. why is the external database store a writer only? It can be an sql reader as well.

          Cheers.

          Show
          Ankit Agarwal added a comment - - edited Hi Petr, Nice work so far. I had another look at the code so far. A few things I noticed :- It would be nice to have tool_log\log\manager::get_writers() , manager::get_enabled_stores(), manager::get_enabled_readers() and manager::get_enabled_writers() , this will also simplify tool_log_setting_managestores::output_html(). core_component is not used in admin/tool/log/classes/plugininfo/logstore.php , but included (or is it included for settings.php?). APIs in admin/tool/log/classes/plugininfo/logstore.php needs phpdocs. What are possible uses of tool_log_setting_managestores::is_related? Won't it create problems when we have plugins with similar names like "external" and "legacyexternal"? tool_log_setting_managestores::output_html has unused vars $CFG and $DB. logstore_database\log\store has apis with missing phpdocs Global namespace specifier is not required for function (\get_config) in logstore_database\log\store::get_config() Following code will be duplicated by most stores. We need to find a way to do this at a higher level: $data = $event->get_data(); $data['other'] = serialize($data['other']); if (CLI_SCRIPT) { $data['origin'] = 'cli'; } else { $data['origin'] = getremoteaddr(); } $data['realuserid'] = \core\session\manager::is_loggedinas() ? $_SESSION['USER']->realuser : null ; Also a lot of methods would have almost identical code for many stores, for example get_name(), get_description(), get_config() etc. May be we should consider using an abstract class or trait here? Also in the same file Behat\Mink\Exception\Exception is included but not used. why is the external database store a writer only? It can be an sql reader as well. Cheers.
          Hide
          Mark Nelson added a comment -

          Hey Petr,

          Thanks for your work on this API.

          admin/tool/log/classes/log/manager.php
          1. Your PHPDoc comments for the class variables $writers and $stores is incorrect - you use the variable $readers for both.
          2. In init() if $this->stores is set you return, which seems fine. However, you call init() in the function process(), which then uses $this->writers later. The same applies to get_readers() which uses $this->readers. Wouldn't it be safer to check that stores, readers and writers is set in init() before returning?
          3. It would be nice if there was a line after the function description and @param for process() and get_store_plugins().
          4. The call to debugging on line 84 should probably be broken down into two lines.
          5. There should be a space after the 'foreach' on line 122.
          admin/tool/log/classes/log/observer.php
          1. I am not convinced a dummy_manager in get_log_mananger() is necessary. Is it in case the user uninstalls the plugin and we need something so it doesn't fail? In that case why don't we just prevent the user from uninstalling the plugin? Or, if there isn't a valid log manager, return false, and whenever get_log_manager is called we handle this case. Just a thought, I can see it being advantageous so when calling get_log_manager you don't need to worry abt the case where no log manager instance is returned.
          2. /** @var \tool_log\log\manager $logmanager */ - Use of inline doc block comments are not allowed.
          admin/tool/log/classes/plugininfo/logstore.php
          1. The namespace 'core_component' is not used any where.
          2. The function get_manage_url() does not contain a space before and after the '=>'.
          3. Why wouldn't you just declare the global variables in settings.php rather than in load_settings()? I know this is what is happening in all other plugins but couldn't it be avoided?
          admin/tool/log/classes/setting_managestores.php
          1. You may want to capitalise the start of your PHPDoc comment for the file.
          2. There is an unnecessary new line between the require_once and the class declaration.
          3. In the function write_setting you have the comment "// do not write any setting" - This comment should be capitalise and end with a full-stop.
          4. For the same function I think there should be a new line between the function description and the @param attribute.
          5. The function output_html declares the global variables $CFG and $DB but they are never used.
          6. When creating the $url variable you do not put a space before and after '=>' in your array.
          7. There should be a space after the foreach.
          8. The same applies to $aurl throughout the function as it did to $url.
          admin/tool/log/db/subplugins.php
          1. No spacing before and after '=>' in your array.
          admin/tool/log/lang/en/tool_log.php
          1. You should capitalise the start of your PHPDoc comment for the file.
          2. What is 'actlogshhdr'? I can't see how this is short for 'Available log stores'.
          admin/tool/log/settings.php
          1. /** @var \tool_log\plugininfo\logstore $plugin */ - Use of inline doc block comments are not allowed.
          admin/tool/log/skodaks_notes.txt
          1. Did you mean to commit this?
          admin/tool/log/store/database/classes/log/store.php
          1. Isn't it possible that we may want to read from an external DB as well?
          2. The call to $db->connect should be placed onto two lines.
          3. If you are going to use double quotes in your debugging message for "Cannot connect to external database: " then there is no need to concatenate the error message, you can simply just include it inside the quotes.
          admin/tool/log/store/database/settings.php
          1. The options array contains some random spacing, there is no need to align the '=>' together. You also have varying spaces between the driver and library in your calls to get_driver_instance().
          2. "// TODO: Localise these settings." - Is there any reason this hasn't been done in this patch? It is only adding language strings right? If there is a valid reason, then it would probably be beneficial to create a tracker issue and link it to this. I see a lot of TODOs in the core code that get ignored and are never dealt with.
          admin/tool/log/store/legacy/classes/event/legacy_logged.php
          1. Why have you not localised the name in this commit? If you are set on not localising this now is there an issue that will?
          2. The only reason I can see that this event exists is so that you can call restore_legacy. This seems kind of hacky to me. Couldn't you remove this event and just create the restore_legacy function in the legacy store?
          admin/tool/log/store/legacy/classes/log/store.php
          1. Is "use Behat\Mink\Exception\Exception;" necessary?
          2. Wouldn't the legacy be a sql_reader, rather than reader?
          3. Are the try catches necessary? We do not normally perform these when performing queries. Also, what if the time specified has no events. Do we really want to throw an error saying we had an issue converting the legacy event data? Wouldn't we just return null? If we can return null if there are no events, then the docs for get_events needs to be updated to reflect this.
          4. I noticed can_access() is not used here. So, if someone was to ignore the function get_readers() and instantiated this class they could then call get_events() which would ignore this check. This shouldn't ever happen, just pointing out that if the function is ignored this could happen.
          admin/tool/log/store/standard/classes/log/store.php
          1. Do you mean !$this->logguests?
          2. I noticed some repeating code "if (CLI_SCRIPT" after reviewing this code. Any way we can avoid having to rewrite this all the time?
          3. Is get_description and get_name necessary? It seems we simply return 'pluginname' and 'pluginname_desc', so why not just return these strings rather than calling these functions so we can avoid repeating this code every time we create another store.
          4. Missing spaces around '=>' in "$extra = array('origin'=>$data->origin, 'realuserid'=>$data->realuserid);"
          admin/tool/log/store/standard/db/install.xml
          1. No new line at end of file.
          admin/tool/log/store/standard/settings.php
          1. Is there any reason you are not localising these settings now?
          admin/tool/log/stores.php
          1. Missing spaces around '=>' in "array('section'=>'managelogging')"
          lib/classes/event/base.php
          1. //debugging('level property is deprecated, use edulevel property instead'); - Why is this being commented out? Are we planning on deprecating this later instead of now? So, you are leaving this as a placeholder? I really think it should be addressed in a new issue rather than leaving these comments in the code. I was wondering why you were changing this name, but I see it is because it is a reserved DB word.
          report/livelogs/index.php
          1. Missing spaces around '=>'.
          2. You have some 'TODO's listed here, but with no explanation of what has to be done. Do you mean to localise these strings? If so, why not do it in this patch?
          report/livelogs/locallib.php
          1. Is there a reason you have not localised the strings now? Is there an issue to add theses?
          2. I am not a fan of adding TODO in the code all over the place, as it seems to get ignored a lot of the time and just never handled. Should we create an issue to deal with this?
          report/livelogs/version.php

          Weird comment spacing. Why not just make it one space?

          All version.php files
          1. Missing full-stop after the comments.
          General
          1. Missing testing instructions.

          Thanks for all the hard work Petr!

          Show
          Mark Nelson added a comment - Hey Petr, Thanks for your work on this API. admin/tool/log/classes/log/manager.php Your PHPDoc comments for the class variables $writers and $stores is incorrect - you use the variable $readers for both. In init() if $this->stores is set you return, which seems fine. However, you call init() in the function process(), which then uses $this->writers later. The same applies to get_readers() which uses $this->readers. Wouldn't it be safer to check that stores, readers and writers is set in init() before returning? It would be nice if there was a line after the function description and @param for process() and get_store_plugins(). The call to debugging on line 84 should probably be broken down into two lines. There should be a space after the 'foreach' on line 122. admin/tool/log/classes/log/observer.php I am not convinced a dummy_manager in get_log_mananger() is necessary. Is it in case the user uninstalls the plugin and we need something so it doesn't fail? In that case why don't we just prevent the user from uninstalling the plugin? Or, if there isn't a valid log manager, return false, and whenever get_log_manager is called we handle this case. Just a thought, I can see it being advantageous so when calling get_log_manager you don't need to worry abt the case where no log manager instance is returned. /** @var \tool_log\log\manager $logmanager */ - Use of inline doc block comments are not allowed. admin/tool/log/classes/plugininfo/logstore.php The namespace 'core_component' is not used any where. The function get_manage_url() does not contain a space before and after the '=>'. Why wouldn't you just declare the global variables in settings.php rather than in load_settings()? I know this is what is happening in all other plugins but couldn't it be avoided? admin/tool/log/classes/setting_managestores.php You may want to capitalise the start of your PHPDoc comment for the file. There is an unnecessary new line between the require_once and the class declaration. In the function write_setting you have the comment "// do not write any setting" - This comment should be capitalise and end with a full-stop. For the same function I think there should be a new line between the function description and the @param attribute. The function output_html declares the global variables $CFG and $DB but they are never used. When creating the $url variable you do not put a space before and after '=>' in your array. There should be a space after the foreach. The same applies to $aurl throughout the function as it did to $url. admin/tool/log/db/subplugins.php No spacing before and after '=>' in your array. admin/tool/log/lang/en/tool_log.php You should capitalise the start of your PHPDoc comment for the file. What is 'actlogshhdr'? I can't see how this is short for 'Available log stores'. admin/tool/log/settings.php /** @var \tool_log\plugininfo\logstore $plugin */ - Use of inline doc block comments are not allowed. admin/tool/log/skodaks_notes.txt Did you mean to commit this? admin/tool/log/store/database/classes/log/store.php Isn't it possible that we may want to read from an external DB as well? The call to $db->connect should be placed onto two lines. If you are going to use double quotes in your debugging message for "Cannot connect to external database: " then there is no need to concatenate the error message, you can simply just include it inside the quotes. admin/tool/log/store/database/settings.php The options array contains some random spacing, there is no need to align the '=>' together. You also have varying spaces between the driver and library in your calls to get_driver_instance(). "// TODO: Localise these settings." - Is there any reason this hasn't been done in this patch? It is only adding language strings right? If there is a valid reason, then it would probably be beneficial to create a tracker issue and link it to this. I see a lot of TODOs in the core code that get ignored and are never dealt with. admin/tool/log/store/legacy/classes/event/legacy_logged.php Why have you not localised the name in this commit? If you are set on not localising this now is there an issue that will? The only reason I can see that this event exists is so that you can call restore_legacy. This seems kind of hacky to me. Couldn't you remove this event and just create the restore_legacy function in the legacy store? admin/tool/log/store/legacy/classes/log/store.php Is "use Behat\Mink\Exception\Exception;" necessary? Wouldn't the legacy be a sql_reader, rather than reader? Are the try catches necessary? We do not normally perform these when performing queries. Also, what if the time specified has no events. Do we really want to throw an error saying we had an issue converting the legacy event data? Wouldn't we just return null? If we can return null if there are no events, then the docs for get_events needs to be updated to reflect this. I noticed can_access() is not used here. So, if someone was to ignore the function get_readers() and instantiated this class they could then call get_events() which would ignore this check. This shouldn't ever happen, just pointing out that if the function is ignored this could happen. admin/tool/log/store/standard/classes/log/store.php Do you mean !$this->logguests? I noticed some repeating code "if (CLI_SCRIPT" after reviewing this code. Any way we can avoid having to rewrite this all the time? Is get_description and get_name necessary? It seems we simply return 'pluginname' and 'pluginname_desc', so why not just return these strings rather than calling these functions so we can avoid repeating this code every time we create another store. Missing spaces around '=>' in "$extra = array('origin'=>$data->origin, 'realuserid'=>$data->realuserid);" admin/tool/log/store/standard/db/install.xml No new line at end of file. admin/tool/log/store/standard/settings.php Is there any reason you are not localising these settings now? admin/tool/log/stores.php Missing spaces around '=>' in "array('section'=>'managelogging')" lib/classes/event/base.php //debugging('level property is deprecated, use edulevel property instead'); - Why is this being commented out? Are we planning on deprecating this later instead of now? So, you are leaving this as a placeholder? I really think it should be addressed in a new issue rather than leaving these comments in the code. I was wondering why you were changing this name, but I see it is because it is a reserved DB word. report/livelogs/index.php Missing spaces around '=>'. You have some 'TODO's listed here, but with no explanation of what has to be done. Do you mean to localise these strings? If so, why not do it in this patch? report/livelogs/locallib.php Is there a reason you have not localised the strings now? Is there an issue to add theses? I am not a fan of adding TODO in the code all over the place, as it seems to get ignored a lot of the time and just never handled. Should we create an issue to deal with this? report/livelogs/version.php Weird comment spacing. Why not just make it one space? All version.php files Missing full-stop after the comments. General Missing testing instructions. Thanks for all the hard work Petr!
          Hide
          CiBoT added a comment -

          Results for MDL-41266

          Show
          CiBoT added a comment - Results for MDL-41266 Remote repository: https://github.com/skodak/moodle.git Remote branch wip_ MDL-37658 _m27_logging to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/329 Warning: The wip_ MDL-37658 _m27_logging branch at https://github.com/skodak/moodle.git has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/329/artifact/work/smurf.html
          Hide
          Petr Škoda added a comment -

          Thanks Mark!

          General notes:
          1. Inline docs blocks with type hints are necessary, the coding style guideline is wrong, it needs to be changed, sorry.
          2. The rules for "=>" are crazy...
          3. I left the TODOs for missing lang strings there, this is not a pull request yet
          4. Long lines are fine imo.

          admin/tool/log/classes/log/manager.php

          • I have commented in the init, checking all properties when they are always set together is unnecessary

          admin/tool/log/classes/log/observer.php

          • The whole point of dummy manager is to have consistent API, depending on you capabilities the manager may not return any readers, it is very consistent now

          admin/tool/log/classes/plugininfo/logstore.php

          • the globals are there for BC, if we decide to remove them we should do it everywhere at the same time imo

          admin/tool/log/skodaks_notes.txt

          • yes I meant it, this was not a pull request, deleted now because all issues were addressed

          admin/tool/log/store/legacy/classes/event/legacy_logged.php

          • this class is hacky but necessary
          • I was considering moving all legacy logging code to this plugin, I am going to work more on it this week

          admin/tool/log/store/legacy/classes/log/store.php

          • hehe, no idea how the mink crawled in!
          • it cannot be sql_reader because the DB table has different columns
          • access control is still in a flux, we will have to review+fix it everywhere

          admin/tool/log/store/standard/classes/log/store.php

          • arrggh, ! fixed
          • this is just a demo, more improvements, refactoring and abstraction is expected

          lib/classes/event/base.php

          • edulevel and debugging is addressed in linked issue

          The issues I did not mentioned above should be all addressed - big thanks!!!

          Show
          Petr Škoda added a comment - Thanks Mark! General notes: 1. Inline docs blocks with type hints are necessary, the coding style guideline is wrong, it needs to be changed, sorry. 2. The rules for "=>" are crazy... 3. I left the TODOs for missing lang strings there, this is not a pull request yet 4. Long lines are fine imo. admin/tool/log/classes/log/manager.php I have commented in the init, checking all properties when they are always set together is unnecessary admin/tool/log/classes/log/observer.php The whole point of dummy manager is to have consistent API, depending on you capabilities the manager may not return any readers, it is very consistent now admin/tool/log/classes/plugininfo/logstore.php the globals are there for BC, if we decide to remove them we should do it everywhere at the same time imo admin/tool/log/skodaks_notes.txt yes I meant it, this was not a pull request, deleted now because all issues were addressed admin/tool/log/store/legacy/classes/event/legacy_logged.php this class is hacky but necessary I was considering moving all legacy logging code to this plugin, I am going to work more on it this week admin/tool/log/store/legacy/classes/log/store.php hehe, no idea how the mink crawled in! it cannot be sql_reader because the DB table has different columns access control is still in a flux, we will have to review+fix it everywhere admin/tool/log/store/standard/classes/log/store.php arrggh, ! fixed this is just a demo, more improvements, refactoring and abstraction is expected lib/classes/event/base.php edulevel and debugging is addressed in linked issue The issues I did not mentioned above should be all addressed - big thanks!!!
          Hide
          Petr Škoda added a comment -

          I have split the livelogs demo report to a new repo: https://github.com/skodak/moodle-report_livelogs

          I am going to clean up the codebase today a bit more so that others may start working on reports...

          Show
          Petr Škoda added a comment - I have split the livelogs demo report to a new repo: https://github.com/skodak/moodle-report_livelogs I am going to clean up the codebase today a bit more so that others may start working on reports...
          Hide
          Rajesh Taneja added a comment -

          Hello Petr,

          While you are looking at this, can you please add default values in legacy_add_to_log() api (/moodle/admin/tool/log/classes/log/manager.php) for info, $cm and $user as they are optional.

          Show
          Rajesh Taneja added a comment - Hello Petr, While you are looking at this, can you please add default values in legacy_add_to_log() api (/moodle/admin/tool/log/classes/log/manager.php) for info, $cm and $user as they are optional.
          Hide
          Petr Škoda added a comment -

          defaults added, thanks

          Show
          Petr Škoda added a comment - defaults added, thanks
          Show
          Marina Glancy added a comment - Please cherry-pick: https://github.com/marinaglancy/moodle/commit/bc5673aa0479c730e34250a1080b6d22c385b528
          Hide
          Petr Škoda added a comment -

          cherry picked, thanks!

          Show
          Petr Škoda added a comment - cherry picked, thanks!
          Show
          Ankit Agarwal added a comment - Please pick https://github.com/ankitagarwal/moodle/commit/3e198141e806df2d971c77d4272674641f49aece cheers
          Hide
          Rajesh Taneja added a comment -

          Hello Petr,

          It seems there are few db fields which need to be replaced for legacy logs.
          https://github.com/rajeshtaneja/moodle/compare/1a07eaa...wip-mdl-39950

          Show
          Rajesh Taneja added a comment - Hello Petr, It seems there are few db fields which need to be replaced for legacy logs. https://github.com/rajeshtaneja/moodle/compare/1a07eaa...wip-mdl-39950
          Hide
          Marina Glancy added a comment -

          Raj, I was working in the same direction in https://github.com/marinaglancy/moodle/compare/skodak:wip_MDL-37658_m27_logging...wip-MDL-41285-master
          But then realised that it's quite impossible to create a single query to apply to both legacylog and log anyway because you can't create any filter by action type

          Show
          Marina Glancy added a comment - Raj, I was working in the same direction in https://github.com/marinaglancy/moodle/compare/skodak:wip_MDL-37658_m27_logging...wip-MDL-41285-master But then realised that it's quite impossible to create a single query to apply to both legacylog and log anyway because you can't create any filter by action type
          Hide
          Petr Škoda added a comment -

          to integrators: the APIs seem pretty stable, submitting for integration but marking as held to satisfy our scrum gods

          Show
          Petr Škoda added a comment - to integrators: the APIs seem pretty stable, submitting for integration but marking as held to satisfy our scrum gods
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (added to current integration, seems to be the major blocked for other tasks)

          Show
          Eloy Lafuente (stronk7) added a comment - (added to current integration, seems to be the major blocked for other tasks)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          oh, my. And cannot things be defined to "default" to old behavior? It seems that there is a looong way before all those get converted. Holding stuff like this for 4 weeks is not an option. Please discuss it there!

          Show
          Eloy Lafuente (stronk7) added a comment - oh, my. And cannot things be defined to "default" to old behavior? It seems that there is a looong way before all those get converted. Holding stuff like this for 4 weeks is not an option. Please discuss it there!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          More yet, how the hell it's possible add_to_log() will lead to any inconsistency? Sounds like a bad thing, as far as we can expect it to continue being used out there for a looong time.

          Better I read the logging specs before shouting... anyway. Ok, keeping this un-integrated (and its list of blocked), until the add_to_log() dependency is analysed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - More yet, how the hell it's possible add_to_log() will lead to any inconsistency? Sounds like a bad thing, as far as we can expect it to continue being used out there for a looong time. Better I read the logging specs before shouting... anyway. Ok, keeping this un-integrated (and its list of blocked), until the add_to_log() dependency is analysed. Ciao
          Hide
          Petr Škoda added a comment -

          We have discussed a new workflow today, we are going to convert issues that are addressed by this branch to subtasks and close them as deferred for now. We will submit it for integration once we have all new reports in place - the reason is we need to make sure the API contains all features necessary for the reporting. Sorry for the confusion...

          Show
          Petr Škoda added a comment - We have discussed a new workflow today, we are going to convert issues that are addressed by this branch to subtasks and close them as deferred for now. We will submit it for integration once we have all new reports in place - the reason is we need to make sure the API contains all features necessary for the reporting. Sorry for the confusion...
          Hide
          Sam Hemelryk added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.
          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
          Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Sam Hemelryk added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao
          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
          CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          Dan Poltawski added a comment -

          Hi Guys,

          I'm afraid that I am not satisfied that this is a good enough API for people to build their logging plugins on top of and so i'm reopening this.

          1. Fundamentally, the reader interface only allows the creation of SQL based log stores. Which gets us no improvement over what we have now:

              /**
               * Fetch records using given criteria.
               *
               * @param string $selectwhere
               * @param array $params
               * @param string $sort
               * @param int $limitfrom
               * @param int $limitnum
               * @return \core\event\base[]
               */
              public function get_events($selectwhere, array $params, $sort, $limitfrom, $limitnum);
          

          1.a) $selectwhere is not an interface which can not be implemented by a file store, or a nosql database - are we really creating an SQL-only interface? We need a higher-level API than that. (In fact I was playing with mongodb this weekend and it would be very well suited to this job, but the API you've created would make it very difficult to implement. See http://docs.mongodb.org/manual/reference/sql-comparison/#select for examples)
          1.b) This is a core Moodle API which you are creating an interface for. Please work a bit harder on documenting what you are creating.

          2.a) I find the organisation of the various interfaces/traits confusing and hard to follow. Perhaps you can explain/justify the split between core and the tool_log?
          2.b) Actually the litmus test of this is whether you can write a simple explanation of how to create a logging plugin and explain where the various interfaces are. Please try that.

          3. Seems like this logging project is determined to be a pinoeer of new php language features in moodle (autoloading, namespaces, traits). I wish that you would start this pioneering work by involving the developer community and help define the rules for how we should use these tools within Moodle, rather than leaving it for the integration team to come in and bring some order after an inconsistent mess has been created (see namespaces). Todays subject is traits and I confess to being ignorant about them having never used them or equivalents in other languages.
          3.a) I would like to see a bit of a justification as to why you decided to go down the traits route here.
          3.b) Your trait implmenentations are calling methods which may not be defined. This is the sort of API direction which I don't like the look of at all. The use of strong interfaces in our APIs brings good benefits for plugin creators. An example is the use of insert_events() in tool_log\helper\writer. Its not defined as an interface anywhere, but only actually created in logstore_database\log\store and logstore_standard\log\store - this sucks. From the look of the docs you can improve this by adding abstract method definitions to the trait (though this has a general smell of bad design about it which I don't like at all).
          3.c) Similarly, comments like '\tool_log\helper\store must be included before using this trait.' do not inspire me with confidence. I'd prefer more concrete interfaces enforced by the code.

          Other little things spoted along the way:

          4. get_log_manager() - why doesn't it deafult back to '\tool_log\log\manager' when LOG_MANAGER_CLASS doesn't exist?
          5. tool_log\log\manager::process() Phpdocs explanation of the method 'Called from the observer only' - not really helpful.
          6. All the strpos in restore_legacy() seem ugly - i'm sure a regualr expression would be cleaner.
          7. I agree with one of the commenters above, there is some duplicated code within the plugins which could do with being abstracted like the CLI handling of ip addresses when inserting events.
          8. Please be careful with recomendations in lang strings (I would suggest to remove the recomendations as they are misleading). e.g.:

          • "Add new records to the legacy log table. It is recommended to disable this for performance reasons."
          • "This setting enables logging of actions by guest account and not logged in users. High profile sites may want to disable this logging for performance reasons. It is recommended to keep this setting enabled on production sites."

          Neither tell the full story, in the first someone with any plugin calling add_to_log() will want to keep this enabled to avoid loosing data.

          Show
          Dan Poltawski added a comment - Hi Guys, I'm afraid that I am not satisfied that this is a good enough API for people to build their logging plugins on top of and so i'm reopening this. 1. Fundamentally, the reader interface only allows the creation of SQL based log stores. Which gets us no improvement over what we have now: /** * Fetch records using given criteria. * * @param string $selectwhere * @param array $params * @param string $sort * @param int $limitfrom * @param int $limitnum * @ return \core\event\base[] */ public function get_events($selectwhere, array $params, $sort, $limitfrom, $limitnum); 1.a) $selectwhere is not an interface which can not be implemented by a file store, or a nosql database - are we really creating an SQL-only interface? We need a higher-level API than that. (In fact I was playing with mongodb this weekend and it would be very well suited to this job, but the API you've created would make it very difficult to implement. See http://docs.mongodb.org/manual/reference/sql-comparison/#select for examples) 1.b) This is a core Moodle API which you are creating an interface for. Please work a bit harder on documenting what you are creating. 2.a) I find the organisation of the various interfaces/traits confusing and hard to follow. Perhaps you can explain/justify the split between core and the tool_log? 2.b) Actually the litmus test of this is whether you can write a simple explanation of how to create a logging plugin and explain where the various interfaces are. Please try that. 3. Seems like this logging project is determined to be a pinoeer of new php language features in moodle (autoloading, namespaces, traits). I wish that you would start this pioneering work by involving the developer community and help define the rules for how we should use these tools within Moodle, rather than leaving it for the integration team to come in and bring some order after an inconsistent mess has been created (see namespaces). Todays subject is traits and I confess to being ignorant about them having never used them or equivalents in other languages. 3.a) I would like to see a bit of a justification as to why you decided to go down the traits route here. 3.b) Your trait implmenentations are calling methods which may not be defined. This is the sort of API direction which I don't like the look of at all . The use of strong interfaces in our APIs brings good benefits for plugin creators. An example is the use of insert_events() in tool_log\helper\writer . Its not defined as an interface anywhere, but only actually created in logstore_database\log\store and logstore_standard\log\store - this sucks. From the look of the docs you can improve this by adding abstract method definitions to the trait (though this has a general smell of bad design about it which I don't like at all). 3.c) Similarly, comments like '\tool_log\helper\store must be included before using this trait.' do not inspire me with confidence. I'd prefer more concrete interfaces enforced by the code. Other little things spoted along the way: 4. get_log_manager() - why doesn't it deafult back to '\tool_log\log\manager' when LOG_MANAGER_CLASS doesn't exist? 5. tool_log\log\manager::process() Phpdocs explanation of the method 'Called from the observer only' - not really helpful. 6. All the strpos in restore_legacy() seem ugly - i'm sure a regualr expression would be cleaner. 7. I agree with one of the commenters above, there is some duplicated code within the plugins which could do with being abstracted like the CLI handling of ip addresses when inserting events. 8. Please be careful with recomendations in lang strings (I would suggest to remove the recomendations as they are misleading). e.g.: "Add new records to the legacy log table. It is recommended to disable this for performance reasons." "This setting enables logging of actions by guest account and not logged in users. High profile sites may want to disable this logging for performance reasons. It is recommended to keep this setting enabled on production sites." Neither tell the full story, in the first someone with any plugin calling add_to_log() will want to keep this enabled to avoid loosing data.
          Hide
          Petr Škoda added a comment - - edited

          here we go again, I thought these discussions are over, the spec was up for review for a long time, now your task as integrator is to look for potential breakages or bugs, not the fundamental design, you should have voiced your concerns much earlier

          Resubmitting and requesting different integrator, ciao

          Show
          Petr Škoda added a comment - - edited here we go again, I thought these discussions are over, the spec was up for review for a long time, now your task as integrator is to look for potential breakages or bugs, not the fundamental design, you should have voiced your concerns much earlier Resubmitting and requesting different integrator, ciao
          Hide
          Dan Poltawski added a comment - - edited

          Petr has argued vocally with me that we about 1a) which is a disagreement on what I think the goals for this logging were.

          I am suggesting that $selectwhere is too freeform and we can come up with something higher level (a more complex version of the select conditions used in get_records perhaps) for systems which aren't sql based for reading. Petr does not agree with this approach at all.

          I would like to know more about why we can't do this - (code examples).

          I also am concerned that we just pass $selectwhere to the external database without any sort of validation. I can easily imagine that people will start passing conditions on MDL tables here too. Note that I think making a more restrictive use of the 'log table' would be necessary - but again, I thought that this was the idea behind the project.

          Show
          Dan Poltawski added a comment - - edited Petr has argued vocally with me that we about 1a) which is a disagreement on what I think the goals for this logging were. I am suggesting that $selectwhere is too freeform and we can come up with something higher level (a more complex version of the select conditions used in get_records perhaps) for systems which aren't sql based for reading. Petr does not agree with this approach at all. I would like to know more about why we can't do this - (code examples). I also am concerned that we just pass $selectwhere to the external database without any sort of validation. I can easily imagine that people will start passing conditions on MDL tables here too. Note that I think making a more restrictive use of the 'log table' would be necessary - but again, I thought that this was the idea behind the project.
          Hide
          Damyon Wiese added a comment -

          I have only had a quick look - not a full look.

          The patch is awesome, but I do think it could be improved - specifically as Dan mentioned to allow different types of storage/reader/report combinations.

          It seems that get_events and get_events_count should be part of the \log\sql_reader - and not part of \log\reader
          and a SQL based report needs to be able to request a sql_reader specifically (or any other type of reader). It's hard to evaluate the flexibility of this API without seeing a complex example of a report. It also could be useful to see an example of a non-sql based store/writer/reader/report to verify the API is flexible enough to allow that.

          Note: that I am only remembering that non-sql scenario was promised way back at the hack fest and have not really been involved with or kept up with the events/logs/reports progress - so if it has been decided that we are not supporting it then fine (as long as it was decided somewhere).

          Show
          Damyon Wiese added a comment - I have only had a quick look - not a full look. The patch is awesome, but I do think it could be improved - specifically as Dan mentioned to allow different types of storage/reader/report combinations. It seems that get_events and get_events_count should be part of the \log\sql_reader - and not part of \log\reader and a SQL based report needs to be able to request a sql_reader specifically (or any other type of reader). It's hard to evaluate the flexibility of this API without seeing a complex example of a report. It also could be useful to see an example of a non-sql based store/writer/reader/report to verify the API is flexible enough to allow that. Note: that I am only remembering that non-sql scenario was promised way back at the hack fest and have not really been involved with or kept up with the events/logs/reports progress - so if it has been decided that we are not supporting it then fine (as long as it was decided somewhere).
          Hide
          Tim Hunt added a comment -

          +1 for Dan's comment https://tracker.moodle.org/browse/MDL-41266?focusedCommentId=271925&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-271925.

          It is no good Petr saying "the spec was up for review for a long time". The events spec was up for a long time. I tried hard to give constructive feedback, and was totally ignored. The only response I got was justifications from the backend team as to why what they had already decided was right, not an attempt to make things better. So when the logging spec came out I just could not summon up the energy to read it and comment.

          Thankfully, there are still people who care, and are prepared to engage. Please stick to your guns Dan, and make sure we end up with an API that does not suck. Thanks.

          Show
          Tim Hunt added a comment - +1 for Dan's comment https://tracker.moodle.org/browse/MDL-41266?focusedCommentId=271925&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-271925 . It is no good Petr saying "the spec was up for review for a long time". The events spec was up for a long time. I tried hard to give constructive feedback, and was totally ignored. The only response I got was justifications from the backend team as to why what they had already decided was right, not an attempt to make things better. So when the logging spec came out I just could not summon up the energy to read it and comment. Thankfully, there are still people who care, and are prepared to engage. Please stick to your guns Dan, and make sure we end up with an API that does not suck. Thanks.
          Hide
          Damyon Wiese added a comment -

          I also do not like the use of traits here. It seems the proper way to use traits, is to define some sort of behaviour (buffered writing would actually be a good example) - then use that trait to pull the behaviour into the class. Instead this seems to be using a trait+interface == a version of an abstract class.

          Show
          Damyon Wiese added a comment - I also do not like the use of traits here. It seems the proper way to use traits, is to define some sort of behaviour (buffered writing would actually be a good example) - then use that trait to pull the behaviour into the class. Instead this seems to be using a trait+interface == a version of an abstract class.
          Hide
          Damyon Wiese added a comment -

          I also am not sure about requiring all log stores to be subplugins of the admin tool that manages them. I know it has been commented to keep the number of plugin types/top level directories down - but this seems like something we may regret later (e.g. if we decide to replace the admin tool). I see that it is can be changed by using a different log_manager - but still - it just doesn't feel right.

          Show
          Damyon Wiese added a comment - I also am not sure about requiring all log stores to be subplugins of the admin tool that manages them. I know it has been commented to keep the number of plugin types/top level directories down - but this seems like something we may regret later (e.g. if we decide to replace the admin tool). I see that it is can be changed by using a different log_manager - but still - it just doesn't feel right.
          Hide
          Martin Dougiamas added a comment -

          After some lengthy discussion in dev chat some of my opinions:

          +1 to define a clear documented SUBSET of SQL that "selectwhere" will support and to make it something that a no-SQL plugin can reasonably support (eg see http://docs.mongodb.org/manual/reference/sql-comparison/#select for rough guide). That's a win-win. SQL-based databases just work, and no-SQL databases can work too (with a bit of translation into native instructions).

          -1 for traits. I think we need to be careful about what we put into Moodle, as a platform for many developers and admins. There needs to be really good reasons for using fairly esoteric new PHP features, and saving a few lines of code is not enough of a reason IMO. If two integrators are having trouble with this new stuff then I think it's a safe bet that the learning curve is exceeding what your average Moodle admin can handle.

          Show
          Martin Dougiamas added a comment - After some lengthy discussion in dev chat some of my opinions: +1 to define a clear documented SUBSET of SQL that "selectwhere" will support and to make it something that a no-SQL plugin can reasonably support (eg see http://docs.mongodb.org/manual/reference/sql-comparison/#select for rough guide). That's a win-win. SQL-based databases just work, and no-SQL databases can work too (with a bit of translation into native instructions). -1 for traits. I think we need to be careful about what we put into Moodle, as a platform for many developers and admins. There needs to be really good reasons for using fairly esoteric new PHP features, and saving a few lines of code is not enough of a reason IMO. If two integrators are having trouble with this new stuff then I think it's a safe bet that the learning curve is exceeding what your average Moodle admin can handle.
          Hide
          Ankit Agarwal added a comment -

          The reason why traits where used where:-

          1. To reduce code duplication.
          2. To help store authors, not to worry about buffering. They can simply say their store supports buffering by importing the trait.
          3. The traits pre-define, most common methods for each type of store (store only/ reader/ writer), so it is much easier to create a store.
          4. Abstract classes cannot be used here, since a store can be implementing multiple interfaces (store/reader/writer/sql_reader)

          The basic concerns from from Dan and Damyon was about using methods, that are not self contained in the traits. We can add abstract methods for such situations.
          However if integrators still don't want traits in, we can do this old fashioned way and let each store define those methods on their own.
          Cheers

          Show
          Ankit Agarwal added a comment - The reason why traits where used where:- To reduce code duplication. To help store authors, not to worry about buffering. They can simply say their store supports buffering by importing the trait. The traits pre-define, most common methods for each type of store (store only/ reader/ writer), so it is much easier to create a store. Abstract classes cannot be used here, since a store can be implementing multiple interfaces (store/reader/writer/sql_reader) The basic concerns from from Dan and Damyon was about using methods, that are not self contained in the traits. We can add abstract methods for such situations. However if integrators still don't want traits in, we can do this old fashioned way and let each store define those methods on their own. Cheers
          Hide
          Michael de Raadt added a comment -

          Can we not have a library of helper functions in a lib file? Maybe I'm being old-fashioned, but that seems simpler than traits.

          Show
          Michael de Raadt added a comment - Can we not have a library of helper functions in a lib file? Maybe I'm being old-fashioned, but that seems simpler than traits.
          Hide
          Ankit Agarwal added a comment -

          No we can not. The traits here are exactly doing what they are designed to do. They are adding additional behaviours to the store, which need it. For example, you can write a basic writer store and use the trait to get benefits of buffering. The methods defined in the store are not something that can be moved out of the class context to a separate lib.php file. There is no requirement for anyone to use them if they don't want to, on the other hand, if you are familiar with traits, it makes life of a store writer much easier. Without them we will end up with lots of duplication among all stores, like we do for sql drivers in core at the moment. Also each third party store will need to implement the whole buffering logic on their own.

          Show
          Ankit Agarwal added a comment - No we can not. The traits here are exactly doing what they are designed to do. They are adding additional behaviours to the store, which need it. For example, you can write a basic writer store and use the trait to get benefits of buffering. The methods defined in the store are not something that can be moved out of the class context to a separate lib.php file. There is no requirement for anyone to use them if they don't want to, on the other hand, if you are familiar with traits, it makes life of a store writer much easier. Without them we will end up with lots of duplication among all stores, like we do for sql drivers in core at the moment. Also each third party store will need to implement the whole buffering logic on their own.
          Hide
          Ankit Agarwal added a comment -

          spec, http://docs.moodle.org/dev/Logging_2#Reading_API has been updated with the details of restrictions for $selectwhere.

          Show
          Ankit Agarwal added a comment - spec, http://docs.moodle.org/dev/Logging_2#Reading_API has been updated with the details of restrictions for $selectwhere.
          Hide
          Petr Škoda added a comment - - edited

          To Dan https://tracker.moodle.org/browse/MDL-41266?focusedCommentId=271925&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-271925 :

          1.a) get_events() methods is not very useful because it returns just list of events. In standard distribution we need it in live report and the other report that allows searching for and filtering of events. It is not useful for participation reports, statistics and pretty much any interesting report because we do not need ten other reports that show individual events, we need to get some more interesting information in reports. This means that most interesting reports are probably going to use the database table directly in custom queries. I do not see a reason for spending lots of time on this if it is not going to be used much.

          1.b) Ankit already added description of the select restrictions into http://docs.moodle.org/dev/Logging_2#Reading_API , I believe this is not a show stopper due to 1.a) because this API is not useful for advanced reports.

          2.a) the organisation of interfaces was done very carefully - the reading and writing of events to log storages is 100% separate. The core contains only the stuff that is used from standard reports, you can for example supply your own logging manager with completely different log writing implementation API and the standard reports are still going to work just fine.

          2.b) This is not about "one log plugin" there are two completely separate APIs involved - first is log writing (defined in tool_log plugin) and log reading interface defined in the core. I wanted to make a simple example of external database log storage without any reading but in the end others implemented the reading support which pretty much spoiled my carefully designed example. I guess we need a new non-sql example here.

          In anycase I do not expect developers start implementing their own log readers and writers unless they already have a custom logging/reporting solution already. We need new interesting reports, not a bazillion of different log storages which have no way of working with event basic reports such as participation and statistics.

          3. Exactly! The idea is to use the new PHP language features consistently/properly in new subsystems because we cannot use them in exsting APIs...

          3.a) Read the traits docs, I suppose you will see that the two independent reading/logging interfaces are the perfect example for trait usage. I was hoping we could introduce it in other areas such as DML drivers.

          3.b) the requirement for insert_events is clearly documented in inline docs. Any way nobody forces you to use the traits, that is the huge difference between extends and uses keywords.

          3.c) Traits are new, traits are different - the resulting code does not feel the same. Your comments seem too much subjective to me, I hope you will reconsider them after you get to use the traits yourself. Or if you really want to try to code it yourself the old way, I would be glad to point out the code constructs that smell silly to me.

          4. The fallback to dummy manager is default intentionally. It allows you to completely disable the logging subsystem. I would consider it a bug if it was falling back to some random or unconfigured logging manager - will not fix

          5. There is nothing wrong with that explanation imho, simply do not ever try to call it directly. Unfortunately PHP is not Java with awesome protected keyword - we cannot restrict access to any public method for specific classes only - will not fix

          6. restore_legacy is very ugly indeed - it will get tons more hacks if you allow use to get this integrated, everything there is 100% self contained and can be fixed/improved later as necessary - this is good design imo

          7. of course there is code duplication - first you design plugins and only after that you start looking for similarities. The point here is that we must make the public API as small as possible which allows addon developers to create some truly innovative log storage and log reader plugins.

          8. Some lang strings suck, I agree. Unfortunately the native English speakers we have in the backend team did not participate for some reason - we need help with fixing of this


          To Damyon https://tracker.moodle.org/browse/MDL-41266?focusedCommentId=271944&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-271944:

          we have spent a lot of time discussion the APIs and I can assure you the way they are they work fine for the reports we have now in Moodle. I believe that your proposal would not allow us to implement them - not a bug imo

          Show
          Petr Škoda added a comment - - edited To Dan https://tracker.moodle.org/browse/MDL-41266?focusedCommentId=271925&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-271925 : 1.a) get_events() methods is not very useful because it returns just list of events. In standard distribution we need it in live report and the other report that allows searching for and filtering of events. It is not useful for participation reports, statistics and pretty much any interesting report because we do not need ten other reports that show individual events, we need to get some more interesting information in reports. This means that most interesting reports are probably going to use the database table directly in custom queries. I do not see a reason for spending lots of time on this if it is not going to be used much. 1.b) Ankit already added description of the select restrictions into http://docs.moodle.org/dev/Logging_2#Reading_API , I believe this is not a show stopper due to 1.a) because this API is not useful for advanced reports. 2.a) the organisation of interfaces was done very carefully - the reading and writing of events to log storages is 100% separate. The core contains only the stuff that is used from standard reports, you can for example supply your own logging manager with completely different log writing implementation API and the standard reports are still going to work just fine. 2.b) This is not about "one log plugin" there are two completely separate APIs involved - first is log writing (defined in tool_log plugin) and log reading interface defined in the core. I wanted to make a simple example of external database log storage without any reading but in the end others implemented the reading support which pretty much spoiled my carefully designed example. I guess we need a new non-sql example here. In anycase I do not expect developers start implementing their own log readers and writers unless they already have a custom logging/reporting solution already. We need new interesting reports, not a bazillion of different log storages which have no way of working with event basic reports such as participation and statistics. 3. Exactly! The idea is to use the new PHP language features consistently/properly in new subsystems because we cannot use them in exsting APIs... 3.a) Read the traits docs, I suppose you will see that the two independent reading/logging interfaces are the perfect example for trait usage. I was hoping we could introduce it in other areas such as DML drivers. 3.b) the requirement for insert_events is clearly documented in inline docs. Any way nobody forces you to use the traits, that is the huge difference between extends and uses keywords. 3.c) Traits are new, traits are different - the resulting code does not feel the same. Your comments seem too much subjective to me, I hope you will reconsider them after you get to use the traits yourself. Or if you really want to try to code it yourself the old way, I would be glad to point out the code constructs that smell silly to me. 4. The fallback to dummy manager is default intentionally. It allows you to completely disable the logging subsystem. I would consider it a bug if it was falling back to some random or unconfigured logging manager - will not fix 5. There is nothing wrong with that explanation imho, simply do not ever try to call it directly. Unfortunately PHP is not Java with awesome protected keyword - we cannot restrict access to any public method for specific classes only - will not fix 6. restore_legacy is very ugly indeed - it will get tons more hacks if you allow use to get this integrated, everything there is 100% self contained and can be fixed/improved later as necessary - this is good design imo 7. of course there is code duplication - first you design plugins and only after that you start looking for similarities. The point here is that we must make the public API as small as possible which allows addon developers to create some truly innovative log storage and log reader plugins. 8. Some lang strings suck, I agree. Unfortunately the native English speakers we have in the backend team did not participate for some reason - we need help with fixing of this To Damyon https://tracker.moodle.org/browse/MDL-41266?focusedCommentId=271944&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-271944: we have spent a lot of time discussion the APIs and I can assure you the way they are they work fine for the reports we have now in Moodle. I believe that your proposal would not allow us to implement them - not a bug imo
          Hide
          Petr Škoda added a comment -

          I personally like the traits use here, it was Ankit who came with this idea and I fully support him here.

          Let me try to explain it in a different way:

          Imagine Victor Frankenstein slicing dead people and putting together really awesome monsters that get the most suitable parts for any given task, let's call these tasks interfaces. The different body parts that can be reused in multiple monsters are called traits in PHP. Not pretty and smells bad, but brutally effective when you want the get the best results quickly.

          Show
          Petr Škoda added a comment - I personally like the traits use here, it was Ankit who came with this idea and I fully support him here. Let me try to explain it in a different way: Imagine Victor Frankenstein slicing dead people and putting together really awesome monsters that get the most suitable parts for any given task, let's call these tasks interfaces. The different body parts that can be reused in multiple monsters are called traits in PHP. Not pretty and smells bad, but brutally effective when you want the get the best results quickly.
          Hide
          Michael de Raadt added a comment -

          I will help with the string improvements.

          Show
          Michael de Raadt added a comment - I will help with the string improvements.
          Hide
          Martin Dougiamas added a comment -

          Petr, thanks for the detailed explanations, that's helpful.

          I've not looked at the code myself but just a thought from an observer: perhaps it could help to have more of these design decisions added in code comments.

          Show
          Martin Dougiamas added a comment - Petr, thanks for the detailed explanations, that's helpful. I've not looked at the code myself but just a thought from an observer: perhaps it could help to have more of these design decisions added in code comments.
          Show
          Petr Škoda added a comment - lang string fixes added in https://github.com/skodak/moodle/commit/726b8161b5e629d884abc4b3b57d708a023b549f
          Hide
          Dan Poltawski added a comment - - edited

          Hi Petr et al,

          To be absolutely clear I am coming at reviewing this code from the point of view of examining what I believe the goals were, considering the code interface from an administrator/plugin developers point of view and compare it to what we have now.

          Now you may chastise me for not being involved in the discussions about the design of this from the very start and sure its a fair criticism. There are not enough hours in the day. But I do not see this as a reason for me to spend 7 or 8 hours reviewing this code and then not raising these points for discussion. My comments were not demands for change, they were points for discussion and justificaiton.

          I completely take the point that the reading and writing interfaces have been split.

          However, your comments still suggest to me that you are not providing a reading API of any value at all. The option made avaialble is to write anywhere, but unless its a database table you can't read from it.

          The core contains only the stuff that is used from standard reports, you can for example supply your own logging manager with completely different log writing implementation API and the standard reports are still going to work just fine.

          I do not see how this can be? Lets imagine a Moodle site which currently struggles with the log table:

          1. They turn off the core loging plugins (this is a problem they have at the moment)
          2. They start logging to their own mongodb plugin
          3. The standard reports will not work??

          In that case they can write their own reports, sure - but the reading is not helped at all by this API. What am I missing?

          In anycase I do not expect developers start implementing their own log readers and writers unless they already have a custom logging/reporting solution already.

          You and I are disagreeing about what should be a goal of this API. I think that the goal of creating this API is to support developers of new solutions for this. Otherwise, what are we actually achieving here? What couldn't previously be achieved by developers using a simple script which simply sucked logs from the current log table, deleting them and inserting them into an external logging system?

          Regarding traits, please don't misunderstand my comments. I do not have a fundamental objection to using them. I am questioning the design (and Ankit has already ackowledged we could tighten up some of the weaker parts of the current design). However..

          Any way nobody forces you to use the traits,

          Again, you (us, we the backend team) are creating an API for other people to use, this is part of our job as core developers of Moodle. We need the code to be understandable for other people to use, we need it documented and we need to communicate with the developer community.

          Show
          Dan Poltawski added a comment - - edited Hi Petr et al, To be absolutely clear I am coming at reviewing this code from the point of view of examining what I believe the goals were, considering the code interface from an administrator/plugin developers point of view and compare it to what we have now. Now you may chastise me for not being involved in the discussions about the design of this from the very start and sure its a fair criticism. There are not enough hours in the day. But I do not see this as a reason for me to spend 7 or 8 hours reviewing this code and then not raising these points for discussion. My comments were not demands for change, they were points for discussion and justificaiton. I completely take the point that the reading and writing interfaces have been split. However, your comments still suggest to me that you are not providing a reading API of any value at all. The option made avaialble is to write anywhere, but unless its a database table you can't read from it. The core contains only the stuff that is used from standard reports, you can for example supply your own logging manager with completely different log writing implementation API and the standard reports are still going to work just fine. I do not see how this can be? Lets imagine a Moodle site which currently struggles with the log table: 1. They turn off the core loging plugins (this is a problem they have at the moment) 2. They start logging to their own mongodb plugin 3. The standard reports will not work?? In that case they can write their own reports, sure - but the reading is not helped at all by this API. What am I missing? In anycase I do not expect developers start implementing their own log readers and writers unless they already have a custom logging/reporting solution already. You and I are disagreeing about what should be a goal of this API. I think that the goal of creating this API is to support developers of new solutions for this. Otherwise, what are we actually achieving here? What couldn't previously be achieved by developers using a simple script which simply sucked logs from the current log table, deleting them and inserting them into an external logging system? Regarding traits, please don't misunderstand my comments. I do not have a fundamental objection to using them. I am questioning the design (and Ankit has already ackowledged we could tighten up some of the weaker parts of the current design). However.. Any way nobody forces you to use the traits, Again, you (us, we the backend team) are creating an API for other people to use, this is part of our job as core developers of Moodle. We need the code to be understandable for other people to use, we need it documented and we need to communicate with the developer community.
          Hide
          Petr Škoda added a comment -

          I am afraid Mongo DB is not the real solution here but a big new set of different problems. The current reading API is open, you may design new readers, but the problem is you need matching reports that can talk to these readers. The reports need to query the readers using some language, the only language we have in Moodle now is good old SQL. SQL is the best general purpose language you can get when writing complex reports especially if you need to use some other existing data that we already have in Moodle tables such as user table, course table or user enrolments.

          The split of read/write operations in Moodle is what many people request, unfortunately it is not possible for the global $DB, but it is possible here and it is expected to be used for logs. Imagine you write logs into a special table that has little indexes and is highly optimised for frequent inserts - this table is not good for reading or complex queries, BUT you can bulk copy the data for example every hour to a different table with your extra indexes that are highly optimised for your database architecture (moodle is not good at indexing because we need to do it cross platform). You may even create a db view which allows you to UNION multiple tables each holding one month of data only. This view can be then be used in any reports that require local Moodle table name.

          You definitely do not need to use any traits in your log reader or writer, the defining API here are interfaces. If you are addon developer you can ignore them completely if you decide to do so, or you may just copy/paste them and forget, or use them.

          We are past the planning stage of logging/reporting API, Mongo DB was never included in the spec. I do not have the slightest idea how to implement a report on top of Mongo DB (without any APIs in between) and I doubt anybody in the backend team does. How are we supposed to design any APIs that work with Mongo DB then? If you want to support it then I am afraid you need to write full spec, write at least sample code and convert all the reports we have at the moment, and finally you need to actually prove it makes sense from the performance point of view...

          Show
          Petr Škoda added a comment - I am afraid Mongo DB is not the real solution here but a big new set of different problems. The current reading API is open, you may design new readers, but the problem is you need matching reports that can talk to these readers. The reports need to query the readers using some language, the only language we have in Moodle now is good old SQL. SQL is the best general purpose language you can get when writing complex reports especially if you need to use some other existing data that we already have in Moodle tables such as user table, course table or user enrolments. The split of read/write operations in Moodle is what many people request, unfortunately it is not possible for the global $DB, but it is possible here and it is expected to be used for logs. Imagine you write logs into a special table that has little indexes and is highly optimised for frequent inserts - this table is not good for reading or complex queries, BUT you can bulk copy the data for example every hour to a different table with your extra indexes that are highly optimised for your database architecture (moodle is not good at indexing because we need to do it cross platform). You may even create a db view which allows you to UNION multiple tables each holding one month of data only. This view can be then be used in any reports that require local Moodle table name. You definitely do not need to use any traits in your log reader or writer, the defining API here are interfaces. If you are addon developer you can ignore them completely if you decide to do so, or you may just copy/paste them and forget, or use them. We are past the planning stage of logging/reporting API, Mongo DB was never included in the spec. I do not have the slightest idea how to implement a report on top of Mongo DB (without any APIs in between) and I doubt anybody in the backend team does. How are we supposed to design any APIs that work with Mongo DB then? If you want to support it then I am afraid you need to write full spec, write at least sample code and convert all the reports we have at the moment, and finally you need to actually prove it makes sense from the performance point of view...
          Hide
          Dan Poltawski added a comment -

          Ignore mongodb, you are missing my point. I am talking about a non-SQL based backend.

          Show
          Dan Poltawski added a comment - Ignore mongodb, you are missing my point. I am talking about a non-SQL based backend.
          Hide
          Petr Škoda added a comment -

          replace mongodb with anything non-sql and see my point

          Show
          Petr Škoda added a comment - replace mongodb with anything non-sql and see my point
          Hide
          Martin Dougiamas added a comment -

          Just one correction: the case of supporting no-SQL databases was there from the beginning. The logging volume is expected to be huge, so we need very fast solutions in order to do useful analytics on logs. However, it's clear that we can't expect these simpler databases to support full SQL, and I don't think anyone is suggesting that.

          I think the restrictions on http://docs.moodle.org/dev/Logging_2#Reading_API for the normal simple reader are good and will allow quite a few useful queries to be made even on noSQL databases. Note also it's not just reports, other Moodle plugins (blocks, local, whatever) might be wanting to do some fast analytics using noSQL databases. For example there is the use case of the little red notification circles showing new items, or real-time analysis of how many current users there are, etc.

          Show
          Martin Dougiamas added a comment - Just one correction: the case of supporting no-SQL databases was there from the beginning. The logging volume is expected to be huge, so we need very fast solutions in order to do useful analytics on logs. However, it's clear that we can't expect these simpler databases to support full SQL, and I don't think anyone is suggesting that. I think the restrictions on http://docs.moodle.org/dev/Logging_2#Reading_API for the normal simple reader are good and will allow quite a few useful queries to be made even on noSQL databases. Note also it's not just reports, other Moodle plugins (blocks, local, whatever) might be wanting to do some fast analytics using noSQL databases. For example there is the use case of the little red notification circles showing new items, or real-time analysis of how many current users there are, etc.
          Hide
          Petr Škoda added a comment -

          The writing to no-sql storages is perfectly possible. The problem here is that people seems to expect a magic API that allows you to do reporting on arbitrary storage which is clearly not possible (feel free to prove me wrong by showing me the changes necessary in our current reports).

          The proposed API is open - that means it allows you to create new core api interface "nosql_reader", the problem here is you need to define how the general plugins are going to process the data stored in it. If we have this interface we may try to emulate sql via no-sql or the other way around or the reports would have to understand both APIs (which should not be difficult by the way).

          Those notification circles can be and probably would be implemented in one of two ways - something like current stats where we take chunks of current log data, process it and store the results in another table. Or you would have a new observer which pushes a pre-processed data into highly optimised no-sql structure and then access it directly during normal page rendering. I do not think it is possible to calculate it on the fly from the raw log data or plain no-sql data store with no custom map-reduce magic.

          Show
          Petr Škoda added a comment - The writing to no-sql storages is perfectly possible. The problem here is that people seems to expect a magic API that allows you to do reporting on arbitrary storage which is clearly not possible (feel free to prove me wrong by showing me the changes necessary in our current reports). The proposed API is open - that means it allows you to create new core api interface "nosql_reader", the problem here is you need to define how the general plugins are going to process the data stored in it. If we have this interface we may try to emulate sql via no-sql or the other way around or the reports would have to understand both APIs (which should not be difficult by the way). Those notification circles can be and probably would be implemented in one of two ways - something like current stats where we take chunks of current log data, process it and store the results in another table. Or you would have a new observer which pushes a pre-processed data into highly optimised no-sql structure and then access it directly during normal page rendering. I do not think it is possible to calculate it on the fly from the raw log data or plain no-sql data store with no custom map-reduce magic.
          Hide
          Martin Dougiamas added a comment -

          Those were just quick examples. If there are better, faster ways we'll use them then. However, it seems clear that occasionally people will want to read straight from logs.

          I don't think you understand that I am actually agreeing with the current approach and just trying to clarify it. No-one is saying that all reports should work with all backends, as far as I can see.

          The key thing is that in the current spec we have two reading APIs already, reader and sql_reader (perhaps better named simple_reader and full_reader to avoid confusion?)

          simple_reader: useful for simplified SQL queries only. All backends must support this method, even noSQL-based ones.
          full_reader: useful for anything complex. Only works on some backends.

          I expect different log reader plugins would declare which of the APIs it supports.

          I expect Moodle code that only needs the simple_reader to just use that. IFF a Moodle plugin needs more and must use full_reader, then I would expect the user/admin to be told that this plugin does not work with the current backend, and to just FAIL gracefully.

          One option for admins could be to use two or more logging backends at the same time, so that they can get the best of both worlds. It might cost them in writing time but pay back in reading time.

          Does this sound OK?

          Show
          Martin Dougiamas added a comment - Those were just quick examples. If there are better, faster ways we'll use them then. However, it seems clear that occasionally people will want to read straight from logs. I don't think you understand that I am actually agreeing with the current approach and just trying to clarify it. No-one is saying that all reports should work with all backends, as far as I can see. The key thing is that in the current spec we have two reading APIs already, reader and sql_reader (perhaps better named simple_reader and full_reader to avoid confusion?) simple_reader: useful for simplified SQL queries only. All backends must support this method, even noSQL-based ones. full_reader: useful for anything complex. Only works on some backends. I expect different log reader plugins would declare which of the APIs it supports. I expect Moodle code that only needs the simple_reader to just use that. IFF a Moodle plugin needs more and must use full_reader, then I would expect the user/admin to be told that this plugin does not work with the current backend, and to just FAIL gracefully. One option for admins could be to use two or more logging backends at the same time, so that they can get the best of both worlds. It might cost them in writing time but pay back in reading time. Does this sound OK?
          Hide
          Petr Škoda added a comment -

          Renaming is perfect for me if there is a consensus, I just need the final interface and method names to update the spec and refactor the code.

          Show
          Petr Škoda added a comment - Renaming is perfect for me if there is a consensus, I just need the final interface and method names to update the spec and refactor the code.
          Hide
          Martin Dougiamas added a comment -

          Dan and Damyon, does what I wrote above make sense, or are you seeing something I missed? (I'm writing on this bug at the exact same time as doing 3 other things so entirely possible)

          Show
          Martin Dougiamas added a comment - Dan and Damyon, does what I wrote above make sense, or are you seeing something I missed? (I'm writing on this bug at the exact same time as doing 3 other things so entirely possible)
          Hide
          Dan Poltawski added a comment - - edited

          simple_reader: useful for simplified SQL queries only. All backends must support this method, even noSQL-based ones.

          It was mentioned to me yesterday that the consensus in the backend scrum was that almost all of our current reports could work with this one too? (Question to backend ppl)

          Show
          Dan Poltawski added a comment - - edited simple_reader: useful for simplified SQL queries only. All backends must support this method, even noSQL-based ones. It was mentioned to me yesterday that the consensus in the backend scrum was that almost all of our current reports could work with this one too? (Question to backend ppl)
          Hide
          Martin Dougiamas added a comment -

          The more that can use it, then the merrier, obviously, let's see code examples.

          Show
          Martin Dougiamas added a comment - The more that can use it, then the merrier, obviously, let's see code examples.
          Hide
          Petr Škoda added a comment - - edited

          live logs - simple event list, can be used with any store that can do simple query for last N events in course
          log viewing - list of events with complex filtering, can use any store that understands the WHERE part of SQL query (restricted syntax)
          participation report - aggregates data per student, class, etc. - requires full SQL queries in local table
          statistics report - needs to copy large chunks of data into temporary table - for now requires local database table, but could be using own observer that increments necessary counters; alternatively there might be external script that sidesteps PHP and gets that data into DB table some other way (please note that the temporary tables could not be used there any more because they are available from our $DB only)

          there are a few other areas such as daily backups that look for specific events in log stores - they require basic WHERE SQL support

          Show
          Petr Škoda added a comment - - edited live logs - simple event list, can be used with any store that can do simple query for last N events in course log viewing - list of events with complex filtering, can use any store that understands the WHERE part of SQL query (restricted syntax) participation report - aggregates data per student, class, etc. - requires full SQL queries in local table statistics report - needs to copy large chunks of data into temporary table - for now requires local database table, but could be using own observer that increments necessary counters; alternatively there might be external script that sidesteps PHP and gets that data into DB table some other way (please note that the temporary tables could not be used there any more because they are available from our $DB only) there are a few other areas such as daily backups that look for specific events in log stores - they require basic WHERE SQL support
          Hide
          Ankit Agarwal added a comment - - edited

          Here are the suggestions from Damyon https://github.com/ankitagarwal/moodle/commit/7b4950386f506db55466b9d22bc1bf6a9d2672c4 . Petr, please cherry-pick the commit.

          Show
          Ankit Agarwal added a comment - - edited Here are the suggestions from Damyon https://github.com/ankitagarwal/moodle/commit/7b4950386f506db55466b9d22bc1bf6a9d2672c4 . Petr, please cherry-pick the commit.
          Hide
          Damyon Wiese added a comment -

          A suggestion for that last commit Ankit - should the writer trait be split into 2 - writer and buffered_writer with writer having an abstract function "write(\core\event\base $event)"

          That way if I do not support buffered writing, I do not need to pull in the extra unused methods (flush) to my class ?

          Show
          Damyon Wiese added a comment - A suggestion for that last commit Ankit - should the writer trait be split into 2 - writer and buffered_writer with writer having an abstract function "write(\core\event\base $event)" That way if I do not support buffered writing, I do not need to pull in the extra unused methods (flush) to my class ?
          Hide
          Damyon Wiese added a comment -

          As noted by Ankit - the writer trait would only have an abstrct method "write" left in it - so no point in having the trait. Renaming the writer to buffered_writer makes sense to me.

          Show
          Damyon Wiese added a comment - As noted by Ankit - the writer trait would only have an abstrct method "write" left in it - so no point in having the trait. Renaming the writer to buffered_writer makes sense to me.
          Hide
          Ankit Agarwal added a comment -
          Show
          Ankit Agarwal added a comment - Renamed writer trait to buffered_writer as suggested, https://github.com/ankitagarwal/moodle/commit/63e43c0fa2e2f969512fb498452fde123651c104
          Hide
          Petr Škoda added a comment -

          Done, thanks Ankit.

          Show
          Petr Škoda added a comment - Done, thanks Ankit.
          Hide
          Tim Hunt added a comment -

          Have you folks seen the recent comments in the forum thread?

          Show
          Tim Hunt added a comment - Have you folks seen the recent comments in the forum thread?
          Hide
          Ankit Agarwal added a comment -
          Show
          Ankit Agarwal added a comment - Petr, can you please cherry-pick this improvement https://github.com/ankitagarwal/moodle/commit/8078b09478413ae14b47247526b4bce36e5738c9
          Hide
          Petr Škoda added a comment -

          thanks Ankit, I have added unit tests there and now I am updating the docs

          Show
          Petr Škoda added a comment - thanks Ankit, I have added unit tests there and now I am updating the docs
          Hide
          Petr Škoda added a comment - - edited

          hi, we have tried to improve the interfaces used from reports, see https://github.com/skodak/moodle/commit/64ecb96f6b3fcbc973cafca0e23830d1c63a0ae4
          feedback welcome, thanks!

          note: the docs were not updated intentionally until we get to some final conclusion necessary for integration request

          Show
          Petr Škoda added a comment - - edited hi, we have tried to improve the interfaces used from reports, see https://github.com/skodak/moodle/commit/64ecb96f6b3fcbc973cafca0e23830d1c63a0ae4 feedback welcome, thanks! note: the docs were not updated intentionally until we get to some final conclusion necessary for integration request
          Hide
          Petr Škoda added a comment -

          There were no objections to the new interface and method names and it looks like majority of HQ devs supports that change. The docs are now updated.

          Submitting for integration, thanks everybody. Fingers crossed...

          Show
          Petr Škoda added a comment - There were no objections to the new interface and method names and it looks like majority of HQ devs supports that change. The docs are now updated. Submitting for integration, thanks everybody. Fingers crossed...
          Hide
          CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          Tim Hunt added a comment -

          Petr, what makes you think "There were no objections to the new interface and method names"? The last two questions in the forum thread have gone unanswered.

          Also, you said a few comments back "note: the docs were not updated intentionally until we get to some final conclusion necessary for integration request", but then you seem to have submitted it for integration without making any further changes to the code (e.g. fixing the comments).

          I hope the integrators are awake.

          Show
          Tim Hunt added a comment - Petr, what makes you think "There were no objections to the new interface and method names"? The last two questions in the forum thread have gone unanswered. Also, you said a few comments back "note: the docs were not updated intentionally until we get to some final conclusion necessary for integration request", but then you seem to have submitted it for integration without making any further changes to the code (e.g. fixing the comments). I hope the integrators are awake.
          Hide
          Petr Škoda added a comment -

          Hi Tim, you are misinterpreting everything I said above: We have discussed the reporting/logging interface names in the last two weeks and it seemed HQ developers came to a conclusion that the reader interfaces and related methods should be renamed, I have created a patch and submitted it here so that others could have a look too. Yesterday I have updated the docs with the new names and submitted for integration. The interfaces should be open enough to implement any type of no-sql readers in the future, but it of course depends on the developers that write reports.

          It seems to me you keep repeating that everything around events/logging/reporting is completely wrong, I personally do not find this a good starting point for a constructive discussion.

          Did you actually try to convert or write some report that is using the new proposed logging API? Did you try to add the new events to existing code? Did you implement any observer for the new events?

          The HQ developers are getting close to finishing the conversion of old events and add_to_log calls to the new events. So far it seems to be having very positive effect both on developers and the codebase:

          • multiple problems in old code were identified and fixed for both events and logging statements
          • there are many new unit tests
          • the automatic checking of event state prevented many regressions
          • all involved developers have learned a lot about the whole codebase
          • the conversion of reports to new logging API is already in progress, it was easier than most devs expected and it starts to show a big potential for future improvements and interesting new report addons

          Of course there were also some things that did not work as planned:
          1/ the renaming of level to edulevel in 2.7 - caused by collision with reserved DB keyword, but in the end it is hopefully easier to understand
          2/ reader->can_access() proved to be too much expensive for normal usage - it may or may not be fully implemented in the future, we might also repurpose it for hiding of anonymous data for example
          3/ event->get_description() - for now it is in English only, it may be converted to lang strings, but again there is a significant cost of fetching all the fancy names from database when viewing events; in any case the current reports do not need to display this information directly because the name + component + context + affected user tell you most of the store
          4/ the docs are still lacking in some areas - we are continually working on that, the most important thing is to get consistent event triggering throughout moodle and describe all the rules and recommendations in docs and developer guides (please note this is close to impossible to do in advance)

          Why did I submit this for integration?

          • The first sample code with current API design was published already some 6 months ago.
          • We have managed to migrate most of the current reports and related functionality to the new APIs without any major problems - this hopefully proves there are no major showstoppers both in implementation and API design.
          • It is either now or next release.

          Ciao and thanks for any constructive feedback.

          Show
          Petr Škoda added a comment - Hi Tim, you are misinterpreting everything I said above: We have discussed the reporting/logging interface names in the last two weeks and it seemed HQ developers came to a conclusion that the reader interfaces and related methods should be renamed, I have created a patch and submitted it here so that others could have a look too. Yesterday I have updated the docs with the new names and submitted for integration. The interfaces should be open enough to implement any type of no-sql readers in the future, but it of course depends on the developers that write reports. It seems to me you keep repeating that everything around events/logging/reporting is completely wrong, I personally do not find this a good starting point for a constructive discussion. Did you actually try to convert or write some report that is using the new proposed logging API? Did you try to add the new events to existing code? Did you implement any observer for the new events? The HQ developers are getting close to finishing the conversion of old events and add_to_log calls to the new events. So far it seems to be having very positive effect both on developers and the codebase: multiple problems in old code were identified and fixed for both events and logging statements there are many new unit tests the automatic checking of event state prevented many regressions all involved developers have learned a lot about the whole codebase the conversion of reports to new logging API is already in progress, it was easier than most devs expected and it starts to show a big potential for future improvements and interesting new report addons Of course there were also some things that did not work as planned: 1/ the renaming of level to edulevel in 2.7 - caused by collision with reserved DB keyword, but in the end it is hopefully easier to understand 2/ reader->can_access() proved to be too much expensive for normal usage - it may or may not be fully implemented in the future, we might also repurpose it for hiding of anonymous data for example 3/ event->get_description() - for now it is in English only, it may be converted to lang strings, but again there is a significant cost of fetching all the fancy names from database when viewing events; in any case the current reports do not need to display this information directly because the name + component + context + affected user tell you most of the store 4/ the docs are still lacking in some areas - we are continually working on that, the most important thing is to get consistent event triggering throughout moodle and describe all the rules and recommendations in docs and developer guides (please note this is close to impossible to do in advance) Why did I submit this for integration? The first sample code with current API design was published already some 6 months ago. We have managed to migrate most of the current reports and related functionality to the new APIs without any major problems - this hopefully proves there are no major showstoppers both in implementation and API design. It is either now or next release. Ciao and thanks for any constructive feedback.
          Hide
          Tim Hunt added a comment -

          Petr, if you want a "constructive discussion", then you need to do your part, and be constructive.

          Is this https://moodle.org/mod/forum/discuss.php?d=235318#p1106549 not a constructive question? Would you like to answer it?

          Show
          Tim Hunt added a comment - Petr, if you want a "constructive discussion", then you need to do your part, and be constructive. Is this https://moodle.org/mod/forum/discuss.php?d=235318#p1106549 not a constructive question? Would you like to answer it?
          Hide
          Damyon Wiese added a comment - - edited

          Ok -

          1. It may be useful to create a new tracker issue to add (e.g.) a simplified sql parser that breaks up a simplified sql expression into it's keywords, operators, groups etc. This code will be the same for any (non-sql) implementing reader. You mentioned you looked at a mongo store - does this code already exist?
          2. Commenting on Tony's concern on the forum about having to choose between a fast performing log storage, and being able to run half the reports in moodle (any using the sql_select_reader interface). This is the same as was dicussed at the last weekly meeting - and this current splitting of the reports is the chosen compromise.
          3. The comments: "\tool_log\helper\store must be included before using this trait" are no longer true (the abstract method just needs to be defined).
          4. Cron - uses legacy cron (how things change in a week!). We should not add new plugin types with support for legacy cron. Scheduled_tasks improve the parallelism and fault tolerance. Can we have a blocker issue for changing this?

          Note - there are no blockers here as long as we agree to the follow up issues.

          Show
          Damyon Wiese added a comment - - edited Ok - It may be useful to create a new tracker issue to add (e.g.) a simplified sql parser that breaks up a simplified sql expression into it's keywords, operators, groups etc. This code will be the same for any (non-sql) implementing reader. You mentioned you looked at a mongo store - does this code already exist? Commenting on Tony's concern on the forum about having to choose between a fast performing log storage, and being able to run half the reports in moodle (any using the sql_select_reader interface). This is the same as was dicussed at the last weekly meeting - and this current splitting of the reports is the chosen compromise. The comments: "\tool_log\helper\store must be included before using this trait" are no longer true (the abstract method just needs to be defined). Cron - uses legacy cron (how things change in a week!). We should not add new plugin types with support for legacy cron. Scheduled_tasks improve the parallelism and fault tolerance. Can we have a blocker issue for changing this? Note - there are no blockers here as long as we agree to the follow up issues.
          Hide
          Ankit Agarwal added a comment -

          We already have an issue to create a Mongo DB store as proof of concept which does SQL emulation - MDL-44382

          Show
          Ankit Agarwal added a comment - We already have an issue to create a Mongo DB store as proof of concept which does SQL emulation - MDL-44382
          Hide
          Damyon Wiese added a comment -

          Added MDL-44431 and MDL-44432. For points 1 and 4 above.

          Show
          Damyon Wiese added a comment - Added MDL-44431 and MDL-44432 . For points 1 and 4 above.
          Hide
          Damyon Wiese added a comment -

          Thanks to all of the backend team, and everyone who gave input to this spec. This has been integrated to master. There are a few follow-up issues as mentioned above.

          Final comment, is that there may need to be some changes to this once the namespaces issue is decided, but there will be new issues created to handle this.

          For anyone watching this issue - although this has been integrated, there will be some API breaking changes before this is released (1 is cron, 2 is potentially namespaces).

          Show
          Damyon Wiese added a comment - Thanks to all of the backend team, and everyone who gave input to this spec. This has been integrated to master. There are a few follow-up issues as mentioned above. Final comment, is that there may need to be some changes to this once the namespaces issue is decided, but there will be new issues created to handle this. For anyone watching this issue - although this has been integrated, there will be some API breaking changes before this is released (1 is cron, 2 is potentially namespaces).
          Hide
          Rajesh Taneja added a comment -

          Hello Damyon,

          This is failing behat, as external db store adds new setting. Can you please pick following patch.
          https://github.com/rajeshtaneja/moodle/compare/745f2cc...wip-mdl-41266

          Above patch has:

          1. Empty option for db driver
          2. Setting default value to none for behat
          Show
          Rajesh Taneja added a comment - Hello Damyon, This is failing behat, as external db store adds new setting. Can you please pick following patch. https://github.com/rajeshtaneja/moodle/compare/745f2cc...wip-mdl-41266 Above patch has: Empty option for db driver Setting default value to none for behat
          Hide
          Damyon Wiese added a comment -

          Hi Raj,

          That commit does not look correct. There is no such setting $CFG->logstore_database.

          Show
          Damyon Wiese added a comment - Hi Raj, That commit does not look correct. There is no such setting $CFG->logstore_database.
          Hide
          Rajesh Taneja added a comment -

          Aha, Thanks Damyon. Fixed.

          Show
          Rajesh Taneja added a comment - Aha, Thanks Damyon. Fixed.
          Hide
          Michael de Raadt added a comment -

          I've started by running unit tests on all DBs. SQLSVR and PostgreSQL are done so far (all clear). Continuing to Oracle and MySQL.

          Andrew has offered to assist with some of the behavioural tests in the sub-tasks.

          Show
          Michael de Raadt added a comment - I've started by running unit tests on all DBs. SQLSVR and PostgreSQL are done so far (all clear). Continuing to Oracle and MySQL. Andrew has offered to assist with some of the behavioural tests in the sub-tasks.
          Hide
          Andrew Davis added a comment -

          MDL-41272 passed.

          Show
          Andrew Davis added a comment - MDL-41272 passed.
          Hide
          Andrew Davis added a comment -

          MDL-41268 passed.

          Show
          Andrew Davis added a comment - MDL-41268 passed.
          Hide
          Andrew Davis added a comment - - edited

          MDL-43931, MDL-43759 and MDL-41269 don't require additional testing.

          I am waiting on additional information for MDL-41271. The only other issue is MDL-39933.

          Show
          Andrew Davis added a comment - - edited MDL-43931 , MDL-43759 and MDL-41269 don't require additional testing. I am waiting on additional information for MDL-41271 . The only other issue is MDL-39933 .
          Hide
          Michael de Raadt added a comment -

          I have run unit tests on all DBs except MySQL (still in progress).

          I have tested MDL-39933 and established external log stores on most DBs so far. There is still no log reading possible yet as the log reports don't allow alternate stores to be used yet.

          Show
          Michael de Raadt added a comment - I have run unit tests on all DBs except MySQL (still in progress). I have tested MDL-39933 and established external log stores on most DBs so far. There is still no log reading possible yet as the log reports don't allow alternate stores to be used yet.
          Hide
          Michael de Raadt added a comment -

          Test result: Passing

          Petr has rewritten the test instructions for MDL-41271 and believes this is covered by unit tests.

          I've run unit tests on all DBs.

          External logging (writing) works locally and remotely. We will need to test log reading when we have the log reports integrated.

          Show
          Michael de Raadt added a comment - Test result: Passing Petr has rewritten the test instructions for MDL-41271 and believes this is covered by unit tests. I've run unit tests on all DBs. External logging (writing) works locally and remotely. We will need to test log reading when we have the log reports integrated.
          Hide
          Michael de Raadt added a comment -

          Thanks to Andrew Davis for his assistance in testing.

          Show
          Michael de Raadt added a comment - Thanks to Andrew Davis for his assistance in testing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For fun: http://www.youtube.com/watch?v=IGENkpaPkgw

          Many thanks for your hard work, this is now part of Moodle!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For fun: http://www.youtube.com/watch?v=IGENkpaPkgw Many thanks for your hard work, this is now part of Moodle! Ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              17 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile