Questions/suggestions about the logging API

Questions/suggestions about the logging API

by Tim Hunt -
Number of replies: 11
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

I finally found the time to read http://docs.moodle.org/dev/Migrating_logging_calls_in_plugins and I have some questions:

1. Why the the class fields use

$this->data[‘something’]

rather than

$this->something

? The second option would be much easier to type.

2. Why is the init method not implemented like

parent::init(‘c’, self::LEVEL_PARTICIPATING, ‘…’);

or

$this->standard_init(‘c’, self::LEVEL_PARTICIPATING, ‘…’);

rather than three lines of code? (Presumably not the former because it involves overriding a method while changing the argument list which gets confusing, but the second version would be more succinct.)

3. Why is ‘crud’ done with literal strings, but ‘edulevel’ done with constants? Consistent APIs are better.

4. I still don’t understand what get_description is for. The description in the page is unsatifying. It seems less good than print_object($event) or class_name($event), while require effort to create.

5. What is the stylistic best practice regarding chaining ->trigger onto the create() call?

6. The contract around add_record_snapshot seem very unclear. Under what circumstances can event receiving code rely on the data being there? If date you need and expect is not there, what do you do?

7. Right at the end of the page, it says “Please never forget that you have to bump the plugin version in order to get all the new events ‘installed’”. Seems like good advice, but why it is right at the end! wink I suggest adding it right at the top when explaining how to define a new event. Oops! “it’s a wiki”. OK, I fixed that.

8. I am still very disappointed in how un-expressive this API is. Logging is conceptually very simple. The amount of code you need to write to log one thing is far larger than necessary. The goal of these changes was to get people to log more, to increase the powder of analytics. I fear the effect of these changes will be the opposite. Logging something is such hard work that people will only log the bare minimum. (Sorry, you are probably bored of hearing me say that.)

9. Can anyone satisfy my curiosity about edulevel? It seems the complete opposite to Moodle’s social constructionist roots. Has it been inspired by some external standard (e.g. TinCan), or did we make it up. In either case, why?

 

Average of ratings: -
In reply to Tim Hunt

Re: Questions/suggestions about the logging API

by Tomasz Muras -
Picture of Core developers Picture of Plugin developers Picture of Plugins guardians Picture of Translators

10. Since get_url() should be implemented, that means one will need at least one event class per entry point (PHP script), is that correct? I wonder if it wouldn't be handier to store URL as an attribute instead of overloading the method.

11. Would it be possible with new logging to implement collecting performance information of running Moodle system? Imagine logging information like memory usage of each page request (somehow) and then storing it in a completely separate log than the rest.

 

cheers,
Tomek

Average of ratings: -
In reply to Tomasz Muras

Re: Questions/suggestions about the logging API

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
10. the get_url() is not returning the URL where was the event triggered, it is the url where you can see the result of the action. Storing of URLs did not work much in the old logging, I do not think we should do that.

11. The new logging is for events only, let's not mix it with performance, please.
Average of ratings: -
In reply to Tim Hunt

Re: Questions/suggestions about the logging API

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
1. this->data makes it easier/faster to verify the data and get it in log storage plugins, all other custom class properties are ignored during logging; I personally do not see anything wrong with it and it seems to work fine.

2. The methods are intentionally implemented so that developers cannot do nasty hacks. Why should it be implemented any other way?

3. I had no personal preference there. But I agree that constants are confusing in external systems.

4. right, get_description() is not very useful for now, it may use lang strings in the future. I do not think it would be a good idea to introduce them right now because they would have to be fixed multiple times which would create unnecessary work for translators.

5. No best practice yet, depends if you need to set custom data and legacy event data and snapshots.

6. The contract is simple, it is a simple database record cache. This is important for performance of event observers.

7. thanks!

8. no comment

9. Originally it was just a general level, but that is a reserved work. So when we were looking for a new word we looked at the actual uses so far and it was mostly - other | participating | managing of course, that is how the edulevel was born. I believe this event property is going to be useful for report developers. The only problem is that sometimes one action might have different level based on how you actually use Moodle.
Average of ratings: -
In reply to Petr Skoda

Re: Questions/suggestions about the logging API

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

Thanks Petr.

Regarding 2 I am still a bit confused. Your stated goal is to ensure "developers cannot do nasty hacks".

but your solution to this is to let developers directly add anything they like to the ->data array, so there is nothing to stop them doing something bad like

$this->data['moodle-hq-sucks'] = true;  // :-)

If, instead, the recommended way to write this code was like my suggestion

$this->standard_init(‘c’, self::LEVEL_PARTICIPATING, '...');

then that would discourage developers from doing nasty hacks. (I am assuming that standard_init is defined in the base class.)

Average of ratings: -
In reply to Tim Hunt

Re: Questions/suggestions about the logging API

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
Did you actually try to convert some events? Apparently you did not because you would know that any extra data is reported in validate_before_trigger() method in debug developer mode.
Average of ratings: -
In reply to Tim Hunt

Re: Questions/suggestions about the logging API

by Dan Poltawski -
  1. I still don’t understand what get_description is for. The description in the page is unsatifying. It seems less good than print_object($event) or class_name($event), while require effort to create.

It is confusing at the moment because of the lack of use. However it it providing a bit more developer utility than your are giving it credit for there. Actually describing the data properties which are available in the event and how the standard properties map is a useful exercise/way of expressing what the event is doing. See for example mod/forum/classes/event/discussion_moved.php. Its useful to express whats happening here when you are writing an event.

Average of ratings: -
In reply to Dan Poltawski

Re: Questions/suggestions about the logging API

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

OK, so it is a debugging tool. I guess it is too late to rename it to a new name that makes the intent clear.

Average of ratings: -
In reply to Tim Hunt

Re: Questions/suggestions about the logging API

by Joseph Rézeau -
Picture of Core developers Picture of Particularly helpful Moodlers Picture of Plugin developers Picture of Testers Picture of Translators

I am currently migrating my add-ons to the new logging/events system, in order to create 2.7 compliant versions. OK, I should have done that quite some time ago, but...

I tend to agree with Tim that "Logging something is such hard work that people will only log the bare minimum." Anyway, I am finding the instructions at http://docs.moodle.org/dev/Migrating_logging_calls_in_plugins#Replacing_.27view.27_events_in_modules useful.

I do not quite see the usefulness of the "Affected user" column. Maybe because my own add-ons are not concerned?

Even from the instructions I do not quite understand the use of add_record_snapshot. I expect if I do not understand that I should not use it.

EDIT.- I almost forgot the main reason for my post. While I am experimenting with the new logging system I really need from time to time to start with a clean slate. How do I completely remove/reset all logs from my local Moodle 2.7 test site?

Joseph

Average of ratings: Useful (1)
In reply to Joseph Rézeau

Re: Questions/suggestions about the logging API

by Ankit Agarwal -

The "Affected user" (relateduserid) column is used to demonstrate the user who is affected by the event at hand.

Consider this example: A teacher X viewes grades of a student Y.

Here Y is the affected user. Obviously if you have an event like course created or something, it doesn't directly affect any user, hence it is not required to set the affected user in such cases.

Moving on to snapshots, snapshot is a mechanism we use to cache certain data, that we think might be useful for the observers. Say there is an observer monitoring "course created event". It is quite possible that it might need access to the $course object and will have to do a database query to get this. To save this extra expense, we add a snapshot of $course to a cache, which can then be used by observers if needed, without any overheads.

To reset all your logs manually for test sites, you can delete the contents of tables "log" and "logstore_standard" , depending on the logstores that you got enabled.

Hope that helps,

Thanks

Average of ratings: Useful (4)
In reply to Tim Hunt

Re: Questions/suggestions about the logging API

by Darko Miletić - 
8. I am still very disappointed in how un-expressive this API is. Logging is conceptually very simple. The amount of code you need to write to log one thing is far larger than necessary. The goal of these changes was to get people to log more, to increase the powder of analytics. I fear the effect of these changes will be the opposite. Logging something is such hard work that people will only log the bare minimum. (Sorry, you are probably bored of hearing me say that.)


I quite agree here with Tim. To jump from add_to_log into this is huge change and not for the better, from developers perspective that is. The API is over engineered and designed with subsequent BI in mind. The pain of bringing this into life is onto us, the developers, and unless explicitly required by project at hand people (me included) will generally avoid this monster. I know this rant does not mean much (if anything at all). This is already approved and introduced in the core, but bring us for once something to make dev life easier. 




Average of ratings: Useful (1)