Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Logs/Log.php
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ public function setLevel(LogLevel $level): self
return $this;
}

public function getPsrLevel(): string
{
return $this->level->toPsrLevel();
}

public function getBody(): string
{
return $this->body;
Expand Down
18 changes: 18 additions & 0 deletions src/Logs/LogLevel.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,24 @@ public function getPriority(): int
return $this->priority;
}

public function toPsrLevel(): string
{
switch ($this->value) {
case 'trace':
return \Psr\Log\LogLevel::NOTICE;
case 'debug':
return \Psr\Log\LogLevel::DEBUG;
case 'warn':
return \Psr\Log\LogLevel::WARNING;
case 'error':
return \Psr\Log\LogLevel::ERROR;
case 'fatal':
return \Psr\Log\LogLevel::CRITICAL;
default:
return \Psr\Log\LogLevel::INFO;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's a good default level here but also this shouldn't really occur

}
Comment thread
cursor[bot] marked this conversation as resolved.
}

private static function getInstance(string $value, int $priority): self
{
if (!isset(self::$instances[$value])) {
Expand Down
2 changes: 1 addition & 1 deletion src/Logs/LogsAggregator.php
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public function add(

// We check if it's a `LogsLogger` to avoid a infinite loop where the logger is logging the logs it's writing
if ($sdkLogger !== null) {
$sdkLogger->log((string) $log->getLevel(), "Logs item: {$log->getBody()}", $log->attributes()->toSimpleArray());
$sdkLogger->log($log->getPsrLevel(), "Logs item: {$log->getBody()}", $log->attributes()->toSimpleArray());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What (I think) @mfb points out is that we copy the log level from the log item instead of logging always as "DEBUG" or "INFO" and mentioning the log level of the log item in the log message.

This was introduced by me I think, this maybe should be updated to mention the log item level in the log message and set our log level to debug (?).

In addition, the comment above this if is outdated and incorrect :) The LogsLogger never made it in but the comment remains 🙈

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I did this very by the book 😎

Then maybe the comment could be updated but no other changes are needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, great - as long as it's by design :)

It's not 100% clear to me but I guess "appropriate" is saying the Sentry log level should be translated to an appropriate level, e.g. log a "fatal" to the console as "critical". I'd also find it reasonable to just print the "fatal" level as part of an "info" log message - either way works for me.

}

$this->logs[] = $log;
Expand Down
27 changes: 27 additions & 0 deletions tests/Logs/LogTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,31 @@ public function testGettersAndSetters(): void
$log->setBody('bar');
$this->assertSame('bar', $log->getBody());
}

/**
* @dataProvider logLevelDataProvider
*/
public function testLogLevelToPsrMapping(LogLevel $logLevel, $expected): void
{
$this->assertSame($expected, $logLevel->toPsrLevel());
}

/**
* @dataProvider logLevelDataProvider
*/
public function testLogAndLogLevelConsistent(LogLevel $level, $expected): void
{
$log = new Log(1.0, '123', $level, 'foo');
$this->assertSame($expected, $log->getPsrLevel());
}

public function logLevelDataProvider(): \Generator
{
yield 'Debug -> Debug' => [LogLevel::debug(), \Psr\Log\LogLevel::DEBUG];
yield 'Trace -> Notice' => [LogLevel::trace(), \Psr\Log\LogLevel::NOTICE];
yield 'Info -> Info' => [LogLevel::info(), \Psr\Log\LogLevel::INFO];
yield 'Warn -> Warning' => [LogLevel::warn(), \Psr\Log\LogLevel::WARNING];
yield 'Error -> Error' => [LogLevel::error(), \Psr\Log\LogLevel::ERROR];
yield 'Fatal -> Critical' => [LogLevel::fatal(), \Psr\Log\LogLevel::CRITICAL];
}
}
Loading