Hm, can you clarify: do you mean to create events for logs? Or to use events to insert into the logging table?

In T389602#10669831, @Ottomata wrote:

Hm, can you clarify: do you mean to create events for logs? Or to use events to insert into the logging table?

There is several options:

1. we could make the log table secondary, and use events to fill it
2. would make the log table a primary entity, and have events that represent changes to it
3. we could keep the log table as a subordinate entity of the page aggregate and emit page events for logged action marked with a special interface (and emit revision related events separately)
4. we could keep the log table as a subordinate, but model all kinds of changes as revision changes, with nothing specific to the log table.

My preference is currently 3, followed by 1. But we should look at all the use cases together.

You could do both 1. and 2.?

log table as a subordinate entity of the page aggregate

Hm, the log table seems like a totally separate thing than a page? It has a lot more than page stuff in it?

Hm, the log table seems like a totally separate thing than a page?

But it logs actions related to pages, and is currently updated along with the page and revision tables, in the same transaction.

It has a lot more than page stuff in it?

Yes, that's true. We'd really have to think if page related logs as separate from other kinds of logs. The fact that they share a db table could be seen as an implementation detail.

I'm not loving this approach. But it matches current behavior well.

Can you say more about the reasoning around wanting to make a LogEntryCreated event a subordinate of PageAggregate?

updated along with the page and revision tables, in the same transaction

That makes sense, and might mean that it is difficult to use an event to update the log table in a deferred update.

But I'm following why that implies that a log is part of the same entity aggregate.

In T389602#10675196, @Ottomata wrote:

Can you say more about the reasoning around wanting to make a LogEntryCreated event a subordinate of PageAggregate?

I don't. LogEntryCreated would be separate, if we need it at all.

But we have events under PageAggregate/PageState that are associated with a log entry (e.g. PageMoved). If the log entry is created by the same code that changes the page, then it's part of the action's outcome and should be modeled as part of the event.

updated along with the page and revision tables, in the same transaction

That makes sense, and might mean that it is difficult to use an event to update the log table in a deferred update.

Well, we could just decide that the log should not be part of the aggregate, and updating the log is merely a consequence of the page change. That would be a slight change in behavior. What is tricky however that different kinds of log entries contain different kinds of data - and that knowledge belongs to the code that makes the change. If we had a LogEventIngress that is responsible for creating log entries based on events it receives, that would make it hard for extensions to define their own log types. Well, maybe not harder than it already is. But the custom logs would not be handled by the ingress object. Extensions would have to implement their own mechanism.

But I'm following why that implies that a log is part of the same entity aggregate.

Because it's that aggregate that creates the log entry. The fact that there is code that does both things in the same transaction means that they belong to the same aggregate. The aggregate is defined as the collection of all things that are changed together atomically.

The aggregate is defined as the collection of all things that are changed together atomically

It is? Huh. There is no reason why a single transaction couldn't update both the user table, and page table, and create a dummy revision, no? Maybe a user rename that also moves the User page?

Even if that kind of thing doesn't happen in practice, I would be surprised if the definition of entity aggregate was everything that happens in one transaction.

LogEventIngress that is responsible for creating log entries based on events it receives, that would make it hard for extensions to define their own log types. Well, maybe not harder than it already is.

Hm, what about:

• Extensions that want to save log entries emit LogEntrySubmittedEvents.
• LogEventIngress listens to all LogEntrySubmittedEvents, and then updates the log table.

If we need to be able to listen to actual log entries created:

• Log entry saving code emits LogEntryCreatedEvents

we could just decide that the log should not be part of the aggregate, and updating the log is merely a consequence of the page change. That would be a slight change in behavior

Understood.

Even if that kind of thing doesn't happen in practice, I would be surprised if the definition of entity aggregate was everything that happens in one transaction.

Not if it just happens to happen in one transaction. Only if the application logic requires it to happen atomically. Currently, that seems to be the case in MediaWiki, but it'S not clear whether it's a requirement or an accident.

Maybe a user rename that also moves the User page?

If there is an integrity requirement, that means that the user page would have to be part of the user aggregate. If the user page is just moved as a consequence of the user rename, then it'S not part of the aggregate. But generally that means it wouldn't happen in the same transaction either - at least it wouldn't if it used our new system.

Extensions that want to save log entries emit LogEntrySubmittedEvents.

It would work I suppose. I'm really like the event-as-instruction pattern, but it's a n option.

Not if it just happens to happen in one transaction. Only if the application logic requires it to happen atomically

Hm. If this is true, then there could be log events that are part the User aggregate when users are renamed? This would be like having a nice structure for log table events that are 'typed' for each kind of potential log event? Some multiple inheritance or trait stuff?

E.g.

LogState
UserState
PageState

PageDeleteLoggedEvent inherits from (somehow) both LogState and PageState?
UserRenameLoggedEvent inherits from (somehow) both LogState and UserState?

Or, perhaps

interface (or trait?) LoggableDomainEvent {
    // returns array of LogEntry created during this event
    getLogEntries(): array; 
}

PageDeletedEvent implements LoggableDomainEvent, ...
UserRenamedEvent implements LoggableDomainEvent, ...

PageDeletedEvent $pageDeletedEvent->getLogEntries()
UserRenamedEvent $userRenamedEvent->getLogEntries()

In T389602#10698884, @Ottomata wrote:

Not if it just happens to happen in one transaction. Only if the application logic requires it to happen atomically

Hm. If this is true, then there could be log events that are part the User aggregate when users are renamed? This would be like having a nice structure for log table events that are 'typed' for each kind of potential log event? Some multiple inheritance or trait stuff?

Indeed! I went for an interface in the PoC: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1128925/3/includes/DomainEvent/LoggableEvent.php

Tagging @Umherirrender because they are apparently the maintainer of Special:Log and the logging table.

Moving a discussion from CR up here to phab.

In the PoC patch, I asked

I'd expect it be possible for an event to have multiple log entries? Would it be better to return a list of them?

@daniel replied:

I'd be surprised if that was the case... it feels wrong, a log entry should repesent one event.

Oh! I think misunderstood what the intention was. I thought you had intended to make it easy for users of an event to get access to associated logs that were created for that event. But, your reply made me realize that you are intending to model a concept of THE log entry for AN event.

Maybe I don't really understand what the logging table is for, but I would expect a log to be a totally separate concept than e.g. a Page or a User entity. So indeed: why is the LogEntry required to be part of the Page aggregate? Perhaps it practically needs to be in part of the same transaction, but I still find it strange that anything in the same transaction is by definition in the same entity aggregate.

What use case do you have in mind?

I guess I don't have one. It just seems quite conceivable that many LogEntries might be associated with an event. There is nothing stopping an emitter from saving multiple log entries. Should they be given the ability to associate those entries with the DomainEvent they emit?

Perhaps, on certain kinds of page edits, they want to insert a few entries into the log? Are there (auto)patroling related use cases? Maybe multiple reverts are created and some extra logs about the revert reasons or aggregate actions taken are logged, as well as the actual revisions created for the revert?

In T389602#10700174, @Ottomata wrote:

Oh! I think misunderstood what the intention was. I thought you had intended to make it easy for users of an event to get access to associated logs that were created for that event. But, your reply made me realize that you are intending to model a concept of THE log entry for AN event.

Both options are on the table. But even when modeling the log entry as part of the entity event (rather than a separate entity), I'd expect there to be one log entry created for an entity change, not more.

Maybe I don't really understand what the logging table is for, but I would expect a log to be a totally separate concept than e.g. a Page or a User entity.

That is my intuition as well, but it's not clear that it would be ok to actually rely on that assumption.

So indeed: why is the LogEntry required to be part of the Page aggregate? Perhaps it practically needs to be in part of the same transaction, but I still find it strange that anything in the same transaction is by definition in the same entity aggregate.

Anything that needs to be part of the transaction would be. Because that need would have to be part of the business logic of the page entity. What else could define that need?

I guess I don't have one. It just seems quite conceivable that many LogEntries might be associated with an event. There is nothing stopping an emitter from saving multiple log entries. Should they be given the ability to associate those entries with the DomainEvent they emit?

It seems like something I'd actively discourage... I don't think we have any action/command creating multiple log entries in the same transaction, and I don't think we should. Io me that would indicate that something is off.

Perhaps, on certain kinds of page edits, they want to insert a few entries into the log? Are there (auto)patroling related use cases?

Currently, edits don't go into the log at all (except for page creation, optionally).

Maybe multiple reverts are created and some extra logs about the revert reasons or aggregate actions taken are logged, as well as the actual revisions created for the revert?

Hm.... maybe? The main purpose of the log is to allow admins to be held accountable for their actions. So I'd expect log entries to correspond to user facting actions, not more fine grained changes. These could be reflected by revisions or tags.

It seems like something I'd actively discourage... I don't think we have any action/command creating multiple log entries in the same transaction, and I don't think we should. Io me that would indicate that something is off.

Okay, if this is true and we can document or enforce this somewhere, then I suppose a singular getLogEntry method would be fine.

In T389602#10699918, @daniel wrote:

Tagging @Umherirrender because they are apparently the maintainer of Special:Log and the logging table.

The logging part is unstewarded as written on https://www.mediawiki.org/wiki/Developers/Maintainers, I have often done changes in that area and listed there as well. But I would not see myself as maintainer to bring the domain events forward here.

Maybe I can help to structure it a bit.

• There are page related log events like mention in the task description (create, move, delete, undelete, page protection, contentmodel change, pagelang change, import, history merge)
  • most are for the whole page,
  • some of them only for specific revisions or list of revisions on that page (like change of visibility with revision-deletion or adding change tags),
  • some for a page reference (title protection is for non-existing pages)
  • page-related includes file-related event like upload or delete of non-current file versions
• User-related log events, the log page is the user page of the target, like block, renameuser, account create (own, auto and for others), user rights (see GetLogTypesOnUserHook)
• Changes to config something, like the managetags logs or abuse filter logs

Additional:

• Page events can have multiple logs
  • file upload creates a revision, a upload log and create log (T290384)
  • move creates a null revision, a move log and a protection move log
• Logs itself can be the target of an action, for example change of visibility with revision-deletion or adding change tags
• Non-public logs like suppress needs a way to have events (See $wgLogRestrictions)
• For Actions acting on pages/revisions that previous inserted also a log, the log part of often not handle correctly, when the event brings both a bit together, this could be handled better in the future (like T19053)

Marking the domain event as "log-adding" sounds like a good idea. But returning a iterable<LogEntry> should be more correct. Maybe a getPrimaryLogEntry is needed to return only the most expected one.

There should be no general event for the insert into the logging table, as there are to many different actions logging things, but it should be possible to listen for all log-adding events.

In T389602#10714453, @Umherirrender wrote:

The logging part is unstewarded as written on https://www.mediawiki.org/wiki/Developers/Maintainers, I have often done changes in that area and listed there as well. But I would not see myself as maintainer to bring the domain events forward here.

Maybe I can help to structure it a bit.

Yes, thank you, that's exactly the information I was hoping for when I tagged you!

We are going to continue to model events for page actions this quarter. It's important to understand their relationship with logs properly. But we probably won't do any work related to the log table as such.

I have another conceptual question I am trying to resolve: is the creation of log entries an integral part of the action (i.e. they have to happen atomically, in the same transaction, and the code that implements the action controls the log entry), or are log entries a consequence of the action (could/should be done afterwards, in a deferred update, and the code implementing the action shouldn't know about them)? The second approach seems better for decoupling, but the first is what we have, and allows us to include more information in the event.

What do you think?

I have another conceptual question I am trying to resolve: is the creation of log entries an integral part of the action (i.e. they have to happen atomically, in the same transaction, and the code that implements the action controls the log entry), or are log entries a consequence of the action (could/should be done afterwards, in a deferred update, and the code implementing the action shouldn't know about them)? The second approach seems better for decoupling, but the first is what we have, and allows us to include more information in the event.

In the second approach, we could lose the deferred update, correct? Working on MW history we found that this happens anyway, but curious if this is generally ok for all the special pages and ways that MW uses the log table.

But why is it that the first approach allows us to include more information? Wouldn't you be able to collect all that same information when you fire the deferred update / event?

In T389602#10721595, @Milimetric wrote:

In the second approach, we could lose the deferred update, correct? Working on MW history we found that this happens anyway, but curious if this is generally ok for all the special pages and ways that MW uses the log table.

Losing a deferred update should be very rare (much more rare than losing a job, which is already one in a million or so). But yes, that's exactly the question - is it ok to lose atomicity?

But why is it that the first approach allows us to include more information? Wouldn't you be able to collect all that same information when you fire the deferred update / event?

The second approach means that the emitter of the original event doesn't know about logs, so it can't represent log entries in the events it fires.
Also, the event object should be complete and immutable when it is fired, it can't be modified later when the domain event executrres.

An ingress object could of course listen to both events and collect the info from both. Bot very pretty, but possible.

I'm working on an overview here: https://docs.google.com/spreadsheets/d/1paYACz7Id2qWMwq4XsjJfcFby8no_jgJpOaDFzk4kMk/edit?gid=0#gid=0