Make dblog entities

Created on 4 January 2015, over 9 years ago
Updated 10 July 2024, 3 days ago

Problem/Motivation

Dblog module hasn't really changed since Drupal 6. The upgrade to Drupal 8 left the dblog Controller with a lot of SQL queries inside. People looking for inspiration in Drupal core, should not use dblog as an example of how to organize the code of a Drupal 9+ project.

There is no way to access dblog entries without directly querying the database, and dblog is usually found in tests where modules want to ensure a given log is present after a certain action. There is no Dblog Entry API either, so everything dblog related is coupled to the table structure. Ideally this problem should be fixed by โœจ Allow kernel tests to fail or expect logged errors Needs work

Proposed resolution

  • Keep using original watchdog table schema, this guarantees sames indexes are created, no performance is list in terms of writing logs into the database.
  • Keep using the Dblog Logger service to insert logs, so no performance is lost when creating logs. Also make the DblogEntry class final, so no contrib module can extend it. This will reduce the need of potential breaking changes.
  • Convert Dblog Entries into content entities. But make these entities read only in the sense they cannot be updated after being created. This is what someone would expects from immutable log entries.
  • Drupal core assumes all content entities can be saved, and jsonapi module has tests for this. This is incompatible with the point mentioned before, so we make the entity type internal to make it invisible to jsonapi. This also means unfortunatelly no jsonapi integration out of the box.
  • Provide an EntityListBuilder to reduce the code of the DblogController. Will also simplify the views integration. and potentially expose the entity via jsonapi or rest modules. Provide a ViewBuilder to render the log details.
  • When uninstalling dblog, dblog entities will be automatically deleted. This is required to be somehow compatible with how dblog worked before the entities conversion, but also to allow uninstall dblog in a production site where logs are created automatically. As this entities are content, DbLogEntryStorage::hasData() returns FALSE when invoked from the ContentUninstallValidator this is a tradeoff to avoid modifying ContentUninstallValidator only for logs.

Regarding BC compatibility:

  • Leave DblogController untouched. Only change references in the the dblog.routing.yml to load the entity collection. By providing a ListBuilder and a ViewBuilder we should be able to replicate the same functionality as currently provided by the dblog module. The current set of automated tests should pass for the new implementation using ListBuilder and ViewBuilder.
  • Direct queries to the watchdog table are still valid, but not recommended anymore.

Remaining tasks

Reviewers needed! All code is done and test are in place.

  • Add the new DblogEntry class [โœ… Done]
  • Add the new DblogEntryStorage class [โœ…Done]
  • Add the View Builder [โœ… Done]
  • Add the List Entity Builder. [โœ… Done]
  • Add the remaining methods the DblogEntryStorageInterface. [โœ… Done]
  • Add new tests (without deleting the existing ones). [โœ… Done]
  • Add Upgrade path for watchdog view [โœ… Done]
  • Get some in depth code reviews. [โŒ in progress]

User interface changes

  • Some internal classes will change, instead of having dblog-error to render the icons, we will rely on numeric values provided by RfcLogLevel. For example severity-4.
  • The backtrace introduced as part of โœจ Display backtrace for logged throwables on log message details page Fixed is displayed under the error message instead to the end of the table

API changes

  • New DblogEntry class. and DblogEntryInterface interface.
  • New DblogEntryStorage class and DblogEntryStorageInteface interface.
  • New DblogEntryAccessControllerHandler class.
  • New DblogViewsData class.
  • New DblogEntryListBuilder class.
  • New FormatMessage service.

Data model changes

None. The watchdog table will remain intact. Column indexes are preserved.

Original report

By @chx: We could make dblog entities with a separate service writing the base data tables for fast writing.

๐Ÿ“Œ Task
Status

Needs review

Version

11.0 ๐Ÿ”ฅ

Component
Database Loggingย  โ†’

Last updated 3 days ago

  • Maintained by
  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina @dagmar
Created by

๐Ÿ‡จ๐Ÿ‡ฆCanada chx

Live updates comments and jobs are added and updated live.
  • Needs framework manager review

    It is used to alert the framework manager core committer(s) that an issue significantly impacts (or has the potential to impact) multiple subsystems or represents a significant change or addition in architecture or public APIs, and their signoff is needed (see the governance policy draft for more information). If an issue significantly impacts only one subsystem, use Needs subsystem maintainer review instead, and make sure the issue component is set to the correct subsystem.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Merge request !2747Make dblog content entities โ†’ (Open) created by dagmar
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Still very interested in this.

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

  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ฆUkraine voleger Ukraine, Rivne

    Added review comments

  • First commit to issue fork.
  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡จ๐Ÿ‡ณChina jungle Chongqing, China
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany mmbk MeiรŸen

    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 โ†’

  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡จ๐Ÿ‡ณChina jungle Chongqing, China

    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.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Gauravvv Delhi, India
  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡จ๐Ÿ‡ณChina jungle Chongqing, China

    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.

  • Status changed to Needs work over 1 year ago
  • 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.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    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.

  • Assigned to dagmar
  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina dagmar Argentina

    I'm back to work on this.

  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina dagmar Argentina

    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 ๐Ÿ“Œ Make dblog entities Postponed , 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.

  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina dagmar Argentina
  • Status changed to Needs work over 1 year ago
  • 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.

  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina dagmar Argentina
  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina dagmar Argentina

    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.

  • Issue was unassigned.
  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina dagmar Argentina

    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 ๐Ÿ› Database primary keys can exceed maximum integer storeable (has actually occurred, in watchdog) Fixed are addressed as the underlying system stills 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 of 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
    2. as this is calling a new API method: ContentEntityTypeInterface;:requireDeleteContentBeforeUninstall which needs some grammar and common sense checking.

    3. Review if all annotations defined in DblogEntry are correct, or if something is missing or redundant.
    4. 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.
  • Status changed to Postponed about 1 year ago
  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina dagmar Argentina

    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: โœจ Provide a way to disable creation of database log entries Needs work

  • Hmm, idk why it doesn't show my picture or link to my profile. That's weird.

  • Merge request !8723Make dblog entities โ†’ (Open) created by dagmar
  • Pipeline finished with Failed
    3 days ago
    Total: 189s
    #220044
  • Pipeline finished with Failed
    3 days ago
    Total: 144s
    #220056
  • Pipeline finished with Failed
    3 days ago
    Total: 206s
    #220059
  • Pipeline finished with Failed
    3 days ago
    Total: 167s
    #220067
  • Pipeline finished with Failed
    3 days ago
    Total: 715s
    #220068
  • Status changed to Needs review 3 days ago
  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina dagmar Argentina

    I found a way to not have to pause the creation of logs and remove the dblog module transparently as usual.

    The approach is not elegant, but it works. When dblog module is asked if has data from the ContentUninstallValidator it replies no... And the module can be uninstalled. Logs are deleted anyways by the dblog_module_preuninstall hook. There is no other way to implement that I'm aware of as the ModuleInstaller::uninstall method does not offer any other opportunity to modify the validations.

    This means this issue is not longer postponed on โœจ Provide a way to disable creation of database log entries Needs work

  • Pipeline finished with Success
    3 days ago
    Total: 743s
    #220106
  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina dagmar Argentina

    dagmar โ†’ changed the visibility of the branch 2401463-dblog-entities-D10 to hidden.

  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina dagmar Argentina

    dagmar โ†’ changed the visibility of the branch 2401463-dblog-entities to hidden.

  • ๐Ÿ‡ฆ๐Ÿ‡ทArgentina dagmar Argentina

    Added โœจ Allow kernel tests to fail or expect logged errors Needs work to the issue summary to, to proper fix the problem with tests. Thanks @joachim

Production build 0.69.0 2024