-
-
Notifications
You must be signed in to change notification settings - Fork 470
fix(logs): use PSR log level when using PSR-3 logger #1907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
b92fba9
6854e03
3a0623a
8e21cd7
a4df205
879d4c7
a59af64
c03ff7d
c5b3633
6b85b87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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()); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://develop.sentry.dev/sdk/telemetry/logs/#other this is by design.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
|
||
There was a problem hiding this comment.
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