PHPLIB-1243: Adapter to pipe ext-mongodb logs to a PSR-3 logger #1158

jmikola · 2023-09-12T04:32:03Z

https://jira.mongodb.org/browse/PHPLIB-1243

src/PsrLogAdapter.php

src/Client.php

src/PsrLogAdapter.php

src/Client.php

GromNaN

Much simpler, and it's easier to understand that the logger is global and not client-specific.

tests/PsrLogAdapterTest.php

src/PsrLogAdapter.php

composer.json

src/PsrLogAdapter.php

jmikola · 2023-09-15T05:33:42Z

src/PsrLogAdapter.php

+     *
+     * @internal
+     */
+    public static function writeLog(int $level, string $domain, string $message): void


Per mongodb/mongo-php-driver#1395 (comment), PHPC defines a mongoc_log() function for internal tests. I'd ultimately like to have it defined only for test builds (PHPC-2293), so I think it still makes sense to use our own writeLog() function here.

We also discussed only caring about logging through PsrLogAdapter for spec compliance, so it shouldn't be necessary to have PHPLIB log directly to a PHPC mechanism (be it the mongodb.debug stream or registered LogSubscriber object(s)).

Since we're only dealing with PSR loggers here, does it make any sense to use the LogSubscriber levels here? The PSR levels are consistent with our logging spec, although the spec defines an additional trace level (how ironic). That said, it's not used so I don't think there's any urgent need to support it.

If we use PSR levels, we'd need a non-optional dependency on psr/log. It may make more sense to define our own levels (exactly consistent with the spec) and then maintain a mapping. This seems to be what Monolog does internally (for both PSR and RFC5424).

I think people will get confused either way. Since PSR log levels are strings and libmongoc log levels are integers, how about accepting both?

This method is internal, people must not use it.

@GromNaN is correct that no users should be calling writeLog() directly. It is only intended for internal use within PHPLIB.

That said, after reading through the Logging spec earlier today, I realized we should probably use those levels directly. My most recent commit introduces constants for the spec levels and changes writeLog() to take those and translate them internally to a PSR level (the only non-standard mapping is spec level trace to PSR level debug).

On a related note, I also changed writeLog() to call the PSR loggers directly instead of forwarding to LogSubscriber::log(), since forwarding would limit what levels PHPLIB could utilize (probably not relevant, but worth noting).

jmikola · 2023-09-15T05:47:35Z

Note: PHPLIB-998 covers full compliance with the Logging spec, so I think there may be more to do here. For instance, logging is supposed to be controlled via environment variables.

I intend to through the spec again and build a list of outstanding features to implement.

alcaeus · 2023-09-18T13:02:16Z

composer.json

@@ -22,6 +22,7 @@
    "require-dev": {
        "doctrine/coding-standard": "^11.1",
        "phpbench/phpbench": "^1.2",
+        "psr/log": "^1.1.4|^2|^3",


We can add this to the suggests section so people know they can use a PSR logger.

Can do. In case you missed @GromNaN's previous comment:

We may mark the dependency as "compatible" in composer, one day. composer/composer#11635

I think this is a moot point. If it was just type hints for the add/removeSubscriber methods, we could stick with suggestions, but PsrLogAdapter references the PSR LogLevel constants directly in a private constant, so we actually depend on psr/log being available.

My most recent commit moves this to a require dependency.

This is still an optional dependency, because logging is an optional feature. You could throw an exception before the PsrLogAdapter class declaration, telling a psr-3 compatible logger needs to be installed.

@GromNaN: I expect at some point PHPLIB will need to write its own log messages for specs, at which point we'd need to introduce the dependency then.

If you feel strongly about making it optional now and deferring the hard requirement until then, I can implement an exception if the PSR classes don't exist.

Ok, let's stick with what's most standard with a required dependency.

src/PsrLogAdapter.php

jmikola · 2023-09-21T02:58:19Z

tests/PsrLogAdapterTest.php

+         * PsrLogAdapter and forwarded to each registered PSR logger.
+         *
+         * Note: it's not possible to test PsrLogAdapter::log() with an invalid
+         * level since mongoc_log() already validates its level parameter. */


Although I ended up removing the previous test that called log() with invalid levels, it was adapted for testWriteLogWithInvalidLevel() below.

alcaeus

LGTM with the repo/branch changes for builds.

.evergreen/config.yml

.github/workflows/tests.yml

src/functions.php

GromNaN · 2023-09-25T10:24:10Z

composer.json

@@ -22,6 +22,7 @@
    "require-dev": {
        "doctrine/coding-standard": "^11.1",
        "phpbench/phpbench": "^1.2",
+        "psr/log": "^1.1.4|^2|^3",


Ok, let's stick with what's most standard with a required dependency.

jmikola · 2023-09-25T18:49:28Z

src/PsrLogAdapter.php

+    public const NOTICE = 5;
+    public const INFO = 6;
+    public const DEBUG = 7;
+    public const TRACE = 8;


Note: I removed the LEVEL_ prefix on these constants, as it conflicted with the definitions on the LogSubscriber interface. PHP didn't seem to care about that, but Psalm did (noticed after correcting the extension version in the Psalm workflow).

These are for internal use only, so I don't particularly care about dropping the prefix. I also considered doing without these and relying on the PSR constants; however, we technically wouldn't have TRACE available (despite it being unlikely a spec would require its use).

jmikola · 2023-09-25T18:50:28Z

src/PsrLogAdapter.php

+
+    private static function getInstance(): self
+    {
+        return self::$instance ??= new self();


@GromNaN: Also refactored this per your previous suggestion in #1158 (comment).

Introduces an internal PsrLogAdapter class to connect PSR-3 loggers with PHPC. The internal class is exposed via addLogger and removeLogger functions. Bumps ext-mongodb dependency to 1.17-dev for PHPC-2180. Permit psr/log 1.x, 2.x, or 3.x for compatibility with PHP 7.4 and 8.0+.

GromNaN · 2025-06-17T11:16:42Z

src/PsrLogAdapter.php

+     * This function is intended for internal use within the library.
+     */
+    public static function writeLog(int $specLevel, string $domain, string $message): void


@alcaeus Why this method isn't @internal?

I don't think that's necessary since the entire class is internal.

jmikola changed the title ~~wip Adapter to pipe ext-mongodb logs to a PSR-3 logger~~ Adapter to pipe ext-mongodb logs to a PSR-3 logger Sep 12, 2023

alcaeus reviewed Sep 12, 2023

View reviewed changes

src/PsrLogAdapter.php Outdated Show resolved Hide resolved

src/PsrLogAdapter.php Outdated Show resolved Hide resolved

src/Client.php Outdated Show resolved Hide resolved

GromNaN reviewed Sep 12, 2023

View reviewed changes

src/PsrLogAdapter.php Outdated Show resolved Hide resolved

src/Client.php Outdated Show resolved Hide resolved

src/Client.php Outdated Show resolved Hide resolved

GromNaN reviewed Sep 13, 2023

View reviewed changes

tests/PsrLogAdapterTest.php Outdated Show resolved Hide resolved

src/PsrLogAdapter.php Outdated Show resolved Hide resolved

src/PsrLogAdapter.php Outdated Show resolved Hide resolved

composer.json Outdated Show resolved Hide resolved

jmikola mentioned this pull request Sep 13, 2023

PHPC-2180: Allow applications to register a log message subscriber mongodb/mongo-php-driver#1395

Merged

GromNaN reviewed Sep 13, 2023

View reviewed changes

src/PsrLogAdapter.php Outdated Show resolved Hide resolved

src/PsrLogAdapter.php Outdated Show resolved Hide resolved

jmikola marked this pull request as ready for review September 14, 2023 13:26

jmikola requested review from GromNaN and alcaeus September 15, 2023 05:29

jmikola commented Sep 15, 2023

View reviewed changes

jmikola force-pushed the phplib-998 branch from 188276b to 31c2654 Compare September 15, 2023 15:02

alcaeus reviewed Sep 18, 2023

View reviewed changes

jmikola changed the title ~~Adapter to pipe ext-mongodb logs to a PSR-3 logger~~ PHPLIB-1243: Adapter to pipe ext-mongodb logs to a PSR-3 logger Sep 20, 2023

jmikola force-pushed the phplib-998 branch 2 times, most recently from c293d6e to 566b1a2 Compare September 21, 2023 02:45

jmikola commented Sep 21, 2023

View reviewed changes

alcaeus self-requested a review September 21, 2023 14:00

jmikola force-pushed the phplib-998 branch from 566b1a2 to decb656 Compare September 21, 2023 18:16

alcaeus reviewed Sep 22, 2023

View reviewed changes

.evergreen/config.yml Outdated Show resolved Hide resolved

.github/workflows/tests.yml Outdated Show resolved Hide resolved

src/functions.php Outdated Show resolved Hide resolved

jmikola force-pushed the phplib-998 branch 3 times, most recently from b7e2800 to 840f450 Compare September 25, 2023 01:23

jmikola mentioned this pull request Sep 25, 2023

PHPLIB-1194: Test log message for non-genuine hosts #1173

Merged

alcaeus approved these changes Sep 25, 2023

View reviewed changes

GromNaN approved these changes Sep 25, 2023

View reviewed changes

jmikola force-pushed the phplib-998 branch 3 times, most recently from eb0edc3 to e976bdc Compare September 25, 2023 18:46

jmikola commented Sep 25, 2023

View reviewed changes

jmikola force-pushed the phplib-998 branch from e976bdc to 4968f8f Compare September 25, 2023 18:52

jmikola force-pushed the phplib-998 branch from 4968f8f to f9c43c3 Compare September 25, 2023 18:56

jmikola merged commit 1acfab0 into mongodb:master Sep 25, 2023

jmikola deleted the phplib-998 branch September 25, 2023 23:43

GromNaN reviewed Jun 17, 2025

View reviewed changes

PHPLIB-1243: Adapter to pipe ext-mongodb logs to a PSR-3 logger #1158

PHPLIB-1243: Adapter to pipe ext-mongodb logs to a PSR-3 logger #1158

Uh oh!

Conversation

jmikola commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GromNaN left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola commented Sep 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola commented Sep 12, 2023 •

edited

Loading

jmikola Sep 21, 2023 •

edited

Loading