Adding JsonFormatter Support to Monolog Handler #268

Spittal · 2024-05-23T16:11:13Z

In the current iteration $record['formatted'] is a string, which is then used later used in an attempt to access message from. I can only imagine this was typo, or an oversight.

I have tested locally and this works now.

welcome · 2024-05-23T16:11:17Z

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

linux-foundation-easycla · 2024-05-23T16:11:20Z

The committers listed above are authorized under a signed CLA.

✅ login: Spittal / name: Jamie Spittal (939ee58, 5a6ff15)

brettmc · 2024-05-24T00:11:48Z

It's not a typo, but it could be a bug, depending on which Monolog formatter you are using. A formatter must be used: https://github.com/Seldaek/monolog/blob/main/src/Monolog/Handler/FormattableHandlerTrait.php#L42 and we configure NormalizerFormatter as the default, I am guessing you've chosen a different formatter?

codecov · 2024-05-24T02:25:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.51%. Comparing base (6cbf493) to head (939ee58).
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #268      +/-   ##
============================================
+ Coverage     83.50%   83.51%   +0.01%     
+ Complexity     1057     1049       -8     
============================================
  Files           104      103       -1     
  Lines          4389     4368      -21     
============================================
- Hits           3665     3648      -17     
+ Misses          724      720       -4

Flag	Coverage Δ
Aws:7.4	`86.02% <ø> (ø)`
Aws:8.0	`85.75% <ø> (ø)`
Aws:8.1	`85.75% <ø> (ø)`
Aws:8.2	`85.75% <ø> (ø)`
Aws:8.3	`85.75% <ø> (ø)`
Context/Swoole:7.4	`0.00% <ø> (ø)`
Context/Swoole:8.0	`0.00% <ø> (ø)`
Context/Swoole:8.1	`0.00% <ø> (ø)`
Context/Swoole:8.2	`0.00% <ø> (ø)`
Context/Swoole:8.3	`0.00% <ø> (ø)`
Instrumentation/CakePHP:8.0	`93.47% <ø> (ø)`
Instrumentation/CakePHP:8.1	`93.47% <ø> (ø)`
Instrumentation/CakePHP:8.2	`93.47% <ø> (ø)`
Instrumentation/CakePHP:8.3	`93.47% <ø> (ø)`
Instrumentation/CodeIgniter:8.0	`75.86% <ø> (ø)`
Instrumentation/CodeIgniter:8.1	`75.86% <ø> (ø)`
Instrumentation/CodeIgniter:8.2	`75.86% <ø> (ø)`
Instrumentation/CodeIgniter:8.3	`75.86% <ø> (ø)`
Instrumentation/ExtAmqp:8.2	`89.58% <ø> (ø)`
Instrumentation/ExtAmqp:8.3	`89.58% <ø> (ø)`
Instrumentation/Guzzle:8.0	`72.60% <ø> (ø)`
Instrumentation/Guzzle:8.1	`72.60% <ø> (ø)`
Instrumentation/Guzzle:8.2	`72.60% <ø> (ø)`
Instrumentation/Guzzle:8.3	`72.60% <ø> (ø)`
Instrumentation/HttpAsyncClient:8.0	`81.33% <ø> (ø)`
Instrumentation/HttpAsyncClient:8.1	`81.33% <ø> (ø)`
Instrumentation/HttpAsyncClient:8.2	`81.33% <ø> (ø)`
Instrumentation/HttpAsyncClient:8.3	`81.33% <ø> (ø)`
Instrumentation/IO:8.2	`75.00% <ø> (ø)`
Instrumentation/IO:8.3	`75.00% <ø> (ø)`
Instrumentation/Laravel:8.0	`65.20% <ø> (ø)`
Instrumentation/Laravel:8.1	`65.20% <ø> (ø)`
Instrumentation/Laravel:8.2	`65.20% <ø> (ø)`
Instrumentation/Laravel:8.3	`65.20% <ø> (ø)`
Instrumentation/MongoDB:7.4	`80.55% <ø> (ø)`
Instrumentation/MongoDB:8.0	`80.55% <ø> (ø)`
Instrumentation/MongoDB:8.1	`80.55% <ø> (ø)`
Instrumentation/MongoDB:8.2	`80.55% <ø> (ø)`
Instrumentation/MongoDB:8.3	`80.55% <ø> (ø)`
Instrumentation/OpenAIPHP:8.1	`86.82% <ø> (ø)`
Instrumentation/OpenAIPHP:8.2	`86.82% <ø> (ø)`
Instrumentation/OpenAIPHP:8.3	`86.82% <ø> (ø)`
Instrumentation/PDO:8.2	`97.46% <ø> (ø)`
Instrumentation/PDO:8.3	`97.46% <ø> (ø)`
Instrumentation/Psr14:8.0	`80.64% <ø> (ø)`
Instrumentation/Psr14:8.1	`80.64% <ø> (ø)`
Instrumentation/Psr14:8.2	`80.64% <ø> (ø)`
Instrumentation/Psr14:8.3	`80.64% <ø> (ø)`
Instrumentation/Psr15:8.0	`93.50% <ø> (ø)`
Instrumentation/Psr15:8.1	`93.50% <ø> (ø)`
Instrumentation/Psr15:8.2	`93.50% <ø> (ø)`
Instrumentation/Psr15:8.3	`93.50% <ø> (ø)`
Instrumentation/Psr16:8.0	`97.50% <ø> (ø)`
Instrumentation/Psr16:8.1	`97.50% <ø> (ø)`
Instrumentation/Psr16:8.2	`97.50% <ø> (ø)`
Instrumentation/Psr16:8.3	`97.50% <ø> (ø)`
Instrumentation/Psr18:8.0	`82.08% <ø> (ø)`
Instrumentation/Psr18:8.1	`82.08% <ø> (ø)`
Instrumentation/Psr18:8.2	`82.08% <ø> (ø)`
Instrumentation/Psr18:8.3	`82.08% <ø> (ø)`
Instrumentation/Psr3:8.0	`63.51% <ø> (ø)`
Instrumentation/Psr3:8.1	`63.51% <ø> (ø)`
Instrumentation/Psr3:8.2	`63.51% <ø> (ø)`
Instrumentation/Psr3:8.3	`63.51% <ø> (ø)`
Instrumentation/Psr6:8.0	`97.61% <ø> (ø)`
Instrumentation/Psr6:8.1	`97.61% <ø> (ø)`
Instrumentation/Psr6:8.2	`97.61% <ø> (ø)`
Instrumentation/Psr6:8.3	`97.61% <ø> (ø)`
Instrumentation/Slim:8.0	`88.14% <ø> (ø)`
Instrumentation/Slim:8.1	`88.88% <ø> (ø)`
Instrumentation/Slim:8.2	`88.88% <ø> (ø)`
Instrumentation/Slim:8.3	`88.88% <ø> (ø)`
Instrumentation/Symfony:8.0	`94.83% <ø> (+1.47%)`	⬆️
Instrumentation/Symfony:8.1	`94.83% <ø> (+1.47%)`	⬆️
Instrumentation/Symfony:8.2	`94.83% <ø> (+1.47%)`	⬆️
Instrumentation/Symfony:8.3	`94.83% <ø> (+1.47%)`	⬆️
Instrumentation/Yii:8.0	`79.82% <ø> (ø)`
Instrumentation/Yii:8.1	`79.82% <ø> (ø)`
Instrumentation/Yii:8.2	`79.82% <ø> (ø)`
Instrumentation/Yii:8.3	`79.82% <ø> (ø)`
Logs/Monolog:7.4	`?`
Logs/Monolog:8.0	`?`
Logs/Monolog:8.1	`?`
Logs/Monolog:8.2	`?`
Logs/Monolog:8.3	`?`
Propagation/ServerTiming:8.0	`100.00% <ø> (ø)`
Propagation/ServerTiming:8.1	`100.00% <ø> (ø)`
Propagation/ServerTiming:8.2	`100.00% <ø> (ø)`
Propagation/ServerTiming:8.3	`100.00% <ø> (ø)`
Propagation/TraceResponse:7.4	`100.00% <ø> (ø)`
Propagation/TraceResponse:8.0	`100.00% <ø> (ø)`
Propagation/TraceResponse:8.1	`100.00% <ø> (ø)`
Propagation/TraceResponse:8.2	`100.00% <ø> (ø)`
Propagation/TraceResponse:8.3	`100.00% <ø> (ø)`
ResourceDetectors/Azure:7.4	`91.66% <ø> (ø)`
ResourceDetectors/Azure:8.0	`91.66% <ø> (ø)`
ResourceDetectors/Azure:8.1	`91.66% <ø> (ø)`
ResourceDetectors/Azure:8.2	`91.66% <ø> (ø)`
ResourceDetectors/Azure:8.3	`91.66% <ø> (ø)`
ResourceDetectors/Container:8.0	`93.02% <ø> (ø)`
ResourceDetectors/Container:8.1	`93.02% <ø> (ø)`
ResourceDetectors/Container:8.2	`93.02% <ø> (ø)`
ResourceDetectors/Container:8.3	`93.02% <ø> (ø)`
Shims/OpenTracing:7.4	`92.99% <ø> (ø)`
Shims/OpenTracing:8.0	`92.99% <ø> (ø)`
Shims/OpenTracing:8.1	`92.99% <ø> (ø)`
Shims/OpenTracing:8.2	`92.99% <ø> (ø)`
Shims/OpenTracing:8.3	`92.99% <ø> (ø)`
Symfony:7.4	`88.43% <ø> (ø)`
Symfony:8.0	`88.20% <ø> (ø)`
Symfony:8.1	`88.20% <ø> (ø)`
Symfony:8.2	`88.20% <ø> (ø)`
Symfony:8.3	`88.20% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cbf493...939ee58. Read the comment docs.

Spittal · 2024-05-30T16:39:36Z

It's not a typo, but it could be a bug, depending on which Monolog formatter you are using. A formatter must be used: https://github.com/Seldaek/monolog/blob/main/src/Monolog/Handler/FormattableHandlerTrait.php#L42 and we configure NormalizerFormatter as the default, I am guessing you've chosen a different formatter?

That's correct, we're using the JsonFormatter. Our exact setup is in the context of Laravel of looks like this.

'otel' => [
    'driver' => 'monolog',
    'handler' => \OpenTelemetry\Contrib\Logs\Monolog\Handler::class,
    'with' => [
        'loggerProvider' => \OpenTelemetry\API\Globals::loggerProvider(),
        'level' => env('LOG_LEVEL', 'debug'),
        'bubble' => true,
    ],
    'replace_placeholders' => true,
    'formatter' => Monolog\Formatter\JsonFormatter::class,
],

I can confirm that with the NormalizerFormatter this works. I'll work on the PR to address this and attempt support with JsonFormatter

Spittal · 2024-05-30T17:57:17Z

Okay I've tested the following formatters

LineFormatter
HtmlFormatter
NormalizerFormatter
ScalarFormatter
JsonFormatter
WildfireFormatter
ChromePHPFormatter
GoogleCloudLoggingFormatter

The others listed here aren't relevant as far as I can tell.

The most recent commit will decode the JSON before populating the LogRecord. This is probably 💯% the desired behaviour, but it's notable that in my case using Loki that I am not required to use the Loki JSON Formatter. If folks really want to have a JSON being received by Loki they can do that in the Otel collector with the Loki exporter I believe.

agoallikmaa · 2024-06-04T09:13:34Z

src/Logs/Monolog/src/Handler.php

@@ -45,7 +45,9 @@ protected function getDefaultFormatter(): FormatterInterface
     */
    protected function write($record): void
    {
-        $formatted = $record['formatted'];
+        $formatted = ($this->getFormatter() instanceof JsonFormatter)


Shouldn't there either be an use for JsonFormatter or fully qualified name?

brettmc · 2024-06-25T01:41:30Z

@Spittal hi, sorry for the slow response. The build is failing on this PR, and it looks like the issue that Ago mentioned above

brettmc · 2024-07-12T00:08:40Z

ping @Spittal

Monolog Handler.php to reference Correct Log Values

5a6ff15

Spittal requested a review from a team May 23, 2024 16:11

Adding JsonFormatter support to Monolog Handler

939ee58

Spittal changed the title ~~Fix Typo in Monolog Handler~~ Adding JsonFormatter Support to Monolog Handler May 30, 2024

agoallikmaa reviewed Jun 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding JsonFormatter Support to Monolog Handler #268

Adding JsonFormatter Support to Monolog Handler #268

Spittal commented May 23, 2024

welcome bot commented May 23, 2024

linux-foundation-easycla bot commented May 23, 2024 •

edited

Loading

brettmc commented May 24, 2024 •

edited

Loading

codecov bot commented May 24, 2024 •

edited

Loading

Spittal commented May 30, 2024 •

edited

Loading

Spittal commented May 30, 2024

agoallikmaa Jun 4, 2024

brettmc commented Jun 25, 2024

brettmc commented Jul 12, 2024

Adding JsonFormatter Support to Monolog Handler #268

Are you sure you want to change the base?

Adding JsonFormatter Support to Monolog Handler #268

Conversation

Spittal commented May 23, 2024

welcome bot commented May 23, 2024

linux-foundation-easycla bot commented May 23, 2024 • edited Loading

brettmc commented May 24, 2024 • edited Loading

codecov bot commented May 24, 2024 • edited Loading

Codecov Report

Spittal commented May 30, 2024 • edited Loading

Spittal commented May 30, 2024

agoallikmaa Jun 4, 2024

Choose a reason for hiding this comment

brettmc commented Jun 25, 2024

brettmc commented Jul 12, 2024

linux-foundation-easycla bot commented May 23, 2024 •

edited

Loading

brettmc commented May 24, 2024 •

edited

Loading

codecov bot commented May 24, 2024 •

edited

Loading

Spittal commented May 30, 2024 •

edited

Loading