-
Notifications
You must be signed in to change notification settings - Fork 116
[Monolog] Add option to directly assign context elements as attributes #467
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #467 +/- ##
============================================
+ Coverage 73.42% 81.63% +8.21%
- Complexity 134 2192 +2058
============================================
Files 11 159 +148
Lines 459 8238 +7779
============================================
+ Hits 337 6725 +6388
- Misses 122 1513 +1391 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 153 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
I think this makes sense. Can the non-default mode (OTEL_PHP_MONOLOG_ATTRIB_MODE=otel) be tested ? |
I copied the behavior from the PSR-3 Instrumentation library, which appears to be the only instance of |
|
Tests added. CI failures are for Symphony, unrelated to this PR. |
| switch (self::$mode) { | ||
| case self::MODE_PSR3: | ||
| if (!is_scalar($attribute)) { | ||
| $attribute = json_encode($attribute); |
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.
Is it worth adding a throw on try/catch with JSON_THROW_ON_ERROR here so that any potential encoding issues can be handled?
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.
How should it be handled? Fallback to serialize?
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.
I also haven't checked if json_encode is even needed or if setAttribute handles it upstream
This PR modifies how Monolog
contextandextrasarrays behave, and adds specific behavior for exceptions.General Attributes
Per PSR-3:
Per Monolog:
Per OTel Spec Logs Data Model:
These descriptions indicate the intent of the context array of PSR-3, and to a lesser extent, the extras array of Monolog, has the same intended function as the Log Record Attributes of OpenTelemetry.
While identifying the need to prevent breaking existing implementations of the OpenTelemetry Monolog Handler, we suggest adding
OTEL_PHP_MONOLOG_ATTRIB_MODEas a runtime configuration option to switch between the current behavior that prioritizes PSR-3 flexibility and safety, and a new behavior that prioritizes the OpenTelemetry spec and SemConv.Examples are in the README.md.
Exceptions
Per PSR-3:
Per the OTel Spec:
Per the OTel SemConv:
These descriptions indicate that exceptions included in the
contextarray under theexceptionkey should be treated differently than other general keys. This behavior is also included in this PR, regardless of the attributes mode.