Make dblog entities

This is needed to be able to integrate with Views Bulk Operations and other modules. I hope someone with knowledge get the time to give us this present :)
At the moment no modules help admin Watchdog entries...

I'm working on this.

A couple remarks on the MR and the testbot is not happy.

Thanks @dagmar :)

Thanks for the initial review @daffie I will move this back to Needs Review when I consider is worth a review. The DblogStorage is incomplete and will have some methods to justify the existence of the class.

Updated issue summary

Review comments need to be addressed.

@voleger Thanks for your help. I'm still working on this, will you let me know when you finish with your modifications?

Sorry, no changes planned.

set proper status

Did you have a look at the way other db loggers like mongodb_watchdog and redis_watchdog do it ? It could be good to have a shared common mechanism for that feature, so that these alternate modules can be smaller and implement only a storage layer.

Did you have a look at the way other db loggers like mongodb_watchdog and redis_watchdog do it ?

@fgm not yet. This is mostly exploratory work at this moment since we do not have any 'non content' and 'non config' entity storage in core right now.

I'm trying to make the tests pass using the current D9 API. But I will take a look later for sure.

Something we will need fixed before implement this. #3233143: DefaultTableMapping expects ContentEntityTypeInterface to work, but this may not be necesary ready for a review.

Most of the failing tests are due #3042467: Support entities that are neither content nor config entities for now I patched jsonapi to skip unsupported entities.

There are a few pending things that I want to address as part of this patch:

1. Add tests for the dblogStorage
2. Add deprecation errors in dblog_filters and other functions
3. Refactor the watchdog view to not rely on some methods of the dblog controller

But for now I think this is ready for a review @daffie @fgm

Did you have a look at the way other db loggers like mongodb_watchdog and redis_watchdog do it ? It could be good to have a shared common mechanism for that feature, so that these alternate modules can be smaller and implement only a storage layer.

@fgm I checked the code of redis_watchdog. With the last set of commits I pushed, this module will have automatic UI if they provide a custom implementation of DblogEntryStorageInterface.

Linked issues resolved by this issue.

I think it is ready for some reviews.

Tested this out on 9.5 and it still applies cleanly

Ran drush updb and the hooks worked created
Code looked clean to me
Only question on the db log page there is an operations column but it's empty.

Not sure how to get a framework manager to review and can't speak on it well enough for a change record.

Don't mind retesting it as this seems like a great feature so will be keeping an eye out! Thanks

Thanks @smustgrave

Please consider the operations column for logs only are available for a very specific set of logs entries, like when creating roles.

what's your thoughts on a default value like "NA"

what's your thoughts on a default value like "NA"

For other listings core does not use NA when no operations are there. I think it should remain empty.

@smustgrave a review on #3233143: DefaultTableMapping expects ContentEntityTypeInterface to work, but this may not be necesary would be useful to unblock some of the dependencies of this issue.

Taking a look now. I can mark this RTBC or should I wait for the change record?

Taking a look now. I can mark this RTBC or should I wait for the change record?

I just updated the change record. I cannot say if you can mark this as RTBC I coded the feature and I'm the dblog subsystem maintainer. I think we need an in depth code review, this is a pretty big change.

Makes sense I took a look and tested locally and nothing seemed to break so I will least give it a vote of RTBC+1

Will post in slack and see if anyone else is interested

I didn't have a very close look, but my gut reaction is that I'm pretty conflicted on this.

The diff here is +2378 -284, its _a lot_ of new code to maintain (and initially review), there's performance to consider, some sites write _a lot_ of log entries (mostly not for good reasons, but it happens, like tons of php notices or so), making it an entity requires a ton of code (as it's a fieldable entity not a content entity, so it gets to reuse very little code but almost all the new code is custom for dblog).

The problem/motivation doesn't seem convincing enough for me to maintain that amount of code. Abstracting the storage could be also done with a regular service, similar to how things like key value store work.

The diff here is +2378 -284, its _a lot_ of new code to maintain (and initially review), there's performance to consider, some sites write _a lot_ of log entries (mostly not for good reasons, but it happens, like tons of php notices or so), making it an entity requires a ton of code (as it's a fieldable entity not a content entity, so it gets to reuse very little code but almost all the new code is custom for dblog).

Glad to see you here @berdir, I was expecting a review from you.

I agree it is a lot of code to review.

write _a lot_ of log entries

Please note the logger system is still just an INSERT INTO, see item one of proposed resolution. Keep using the Dblog Logger service to insert logs, so no performance is lost when creating logs

The problem/motivation doesn't seem convincing enough for me to maintain that amount of code. Abstracting the storage could be also done with a regular service, similar to how things like key value store work.

Could you elaborate this a bit more?

Just following up on this one.

I did some profiling. Test scenarios:

1. Install the site using drush.

2. Load a page not found as soon the site is installed.

3. Load a second page different page not found.

So I think we need to dig a bit deeper on this massive performance on uncached pages.

I will keep the needs profiling tag to see if I can fix this issue and provide new data.

I ran the profiling tests again with more detail this time. It seems I was comparing incorrect data. Here are my findings. In all cases Left is drupal 9.4 using regular watchdog entries. Right dblog entities. All test were made using the Standard profile.

Visit homepage after clearing the cache with drush cr

A page not found after visiting the homepage (cache warm scenario)

Another page not found just after cleaning cache

Visit the logs overview page - (cache warm scenario)

Visit the logs overview page just after cleaning cache

Display an individual log event - (cache warm scenario)

That is very neat! If I'm understanding the numbers correctly it's slightly better in a lot of scenarios.

May be out of my league to fully mark it but +1 for RTBC

I really like the idea of this issue but was surprised to see that dblog entries are not content entities here. Did not really find an explanation for that here, is

Dblog entries could be this example, as they are practically a simplified version of content entities. Without revisions, without translations, without dynamic fields.

from the issue summary it?

If so, I'm not sure I'm conviced to be honest. I would agree that it does not make sense for dblog entries to follow the lead of nodes, terms, etc. by extending EditorialContentEntityBase but in my opinion it would still make sense for it to be a content entity. It's perfectly fine to not opt-in to revisions or translations and by not declaring a field_ui_base_route, you are really disabling dynamic fields for all intents and purposes. That would achieve the "simplified" part as far as I can tell but allow a much greater amount of code re-use.

Or are there other reasons that aren't explained here (or that are and I have overlooked, in which case: sorry!).

@tstoeckler thanks for your feedback (and @berdir too). I had to do some tests to remember why I took the decision of not going with regular content entities.

It seems all boils down to the plan to make the new entity type compatible with the legacy approach of using $log->severity as a StdClass object.

Today I reflected about this architecture decision and I think it was incorrect. This is a new API, we have the chance to define the interface and let others use it properly.

I will take some days to refactor the codebase. I did some quick tests today and indeed it is possible to extend ContentEntityBase. Initial refactor work attached.

Alright, commit 12a875fb has a working patch for making dblog content entities, against Drupal 9.4.x. I have to say I'm much more comfortable with the shape of the code right now. Much less code duplication. And half of the lines of this patch are tests. Thanks @Berdir and @tstoeckler for pushing in that direction.

Some pending follow ups:

• I don't see this happening for Drupal 9, so most likely continue this for Drupal 10.x or 10.1.x
• Check failing tests for D10 (if any). There is a new branch for this https://git.drupalcode.org/project/drupal/-/merge_requests/2747
• There are a few empty methods of the ViewBuilder that can be removed.
• Review if the no cache strategy is correct. In my opinion logs shouldn't be cached at all. To prevent flooding the cache table with garbage
• Review new documentation, I'm not a native English speaker
• If you wonder why there is a new dblog.formatter, the reason is because this will allow to make logs not depending of translations. See #2843563: Allow to choose if logs should be translated or not
• New profiling are required, since this is not the same code I profiled time ago

I won't be able to work on any Drupal issues anymore for the next 4~6 months (new baby incoming) so I'm leaving this for someone else to continue and do the follow ups. 👋

Still very interested in this.

Wonder if you could provide a good way for one to test this?

Added review comments

The failing test with postgres is not generated by this patch, it already fails with the 10.1.x-dex branch https://www.drupal.org/node/3060/qa

NW for #75

1. Are chosen names for the DblogStorageInterface methods are good enough?

No opinion.

2. Are we ok with forbidding individual deletion of entries?

Any reason for this? -1.

3. Are we ok with preventing saving an DblogEntry using save() and suggests instead use the Dblog logger?

Make sense.

Per the test result of DbLogEntryTest::testSerialization() #75 may not be an issue anymore. Probably, it's not an issue in newer versions of PostgreSQL.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Left some comments on the MR. More than profiling I am wondering if the indexes are correct (i.e. an EXPLAIN on some common queries would be useful) and whether the schema matches between existing sites and a new install.

I'm back to work on this.

I updated the issue summary to answer @catch concerns regarding indexes. I ran a `DESCRIBE watchdog;` command before and after installing the site using this patch, and the table structure remains the same.

I guess this also is valid for the issue #3081144: Database primary keys can exceed maximum integer storeable (has actually occurred, in watchdog), if we fix this in the dblog.install module, the changes will also apply to dblog as entities.

I also addressed the comments about dblog exception in ContentUninstallValidator. Based on my current understanding of the module_installer logic, there is no way for dblog to by pass this content validator exception, so I had to modify it any way. I tried to make it as simple and clear possible, and not only for dblog.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I updated the issue summary based on the latest reviews assuming the proposed method names are ok. Removing needs profiling as I did the profiling tests in #61.

I went over all the pending open threads of the merge request and closed most of them. I left a few open to make sure the provided answer is enough for the original reviewers.

I also checked some concerns I had related caching. It seems entities are not cached but routes are (example route:[language]=en:/admin/reports/dblog/event/10) in cache_data table. This is also happening for the current non entity implementation so I will not attempt to fix this as part of this issue. Maybe is not even an issue.

Most of the concerns regarding performance, indexing and future compatibility with #3081144: Database primary keys can exceed maximum integer storeable (has actually occurred, in watchdog) are addressed as the underlying system still uses the same database scheme and insert queries.

My general feeling about the code (as the current dblog maintainer) is: this is pretty much ready to be in. There is some room of improvement in the DblogEntryViewBuilder, but probably qualifies for a follow up. And comparing this with the current code of the dblog module I would say this is a big improvement in code quality and alignment with Drupal standards. And will also allow to close several postponed issues for more than a few years now.

The remaining steps in my opinion are:

1. Reviewing the latest changes to ContentUninstallValidator::validate

as this is calling a new API method: ContentEntityTypeInterface;:requireDeleteContentBeforeUninstall which needs some grammar and common sense checking.

2. Review if all annotations defined in DblogEntry are correct, or if something is missing or redundant.
3. Get some last code review round paying specially attention to the high level picture of this. I already did this a few times, but I wrote most of the code, and doesn't feel right to assume it is right.

I had more time to think about item 1. I think we should not make an exception for dblog entries. After researching a bit why the content validator was created it seems a bit dangerous to bypass the checks.

Concerns about disabling the dblog module in a production environment are still valid though. I created this to take care of this functionality: #3354081: Provide a way to disable creation of database log entries

Comment no longer applies