implement events v1.32 #1291
implement events v1.32 #1291
Conversation
implement the events api + sdk per spec v1.32: - event logger is now only retrievable via an event logger provider - domain attribute for events is removed - events accept a subset of logrecord params, rather than an entire logrecord
| $context && $logRecord->setContext($context); | ||
| $logRecord->setSeverityNumber($severityNumber ?? Severity::INFO); | ||
|
|
||
| $this->logger->emit($logRecord); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1291 +/- ##
============================================
+ Coverage 74.00% 74.02% +0.02%
- Complexity 2367 2384 +17
============================================
Files 347 352 +5
Lines 7077 7131 +54
============================================
+ Hits 5237 5279 +42
- Misses 1840 1852 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
src/API/Logs/LogRecord.php
Outdated
| @@ -42,9 +42,9 @@ public function setContext(?ContextInterface $context = null): self | |||
| /** | |||
| * @see https://opentelemetry.io/docs/reference/specification/logs/data-model/#field-severitynumber | |||
| */ | |||
| public function setSeverityNumber(int $severityNumber): self | |||
| public function setSeverityNumber(Severity $severityNumber): self | |||
There was a problem hiding this comment.
Might want to keep Severity|int here to preserve BC.
There was a problem hiding this comment.
Since we've removed emitEvent(LogRecord), there shouldn't be any direct usage of this class outside of log appenders (the monolog one in contrib is the only one I'm aware of). So, I don't think this is a critical BC to maintain...?
There was a problem hiding this comment.
It has very likely low/no impact (as LogRecord::setSeverityNumber(Psr::serverityNumber($...)) stays compatible); but it is also quite trivial to keep BC. IMO we should preserve BC for the low chance that someone is using the logs API w/ a non-PSR-3 logger.
There was a problem hiding this comment.
Fair enough, reverted to maintain BC. I also deprecated Psr::severityNumber in favour of moving that logic into the enum itself.
This reverts commit 7108229. Also, move PSR-3 mapping into the Severity enum.
* implement events v1.32 implement the events api + sdk per spec v1.32: - event logger is now only retrievable via an event logger provider - domain attribute for events is removed - events accept a subset of logrecord params, rather than an entire logrecord * convert severity to a backed enum * lint * remove instead of deprecating logEvent, mark Logger constructor as internal * make severity an enum only * event attributes to iterable * inject ClockInterface, add CachedInstrumentation, update examples * set correct defaults for events * test coverage * Revert "make severity an enum only" This reverts commit 7108229. Also, move PSR-3 mapping into the Severity enum. * event attributes to iterable * apply review feedback
implement the events api + sdk per spec v1.32:
todo: the spec is a little unclear on exactly how an EventLoggerProvider should access the Logger that it uses. Since the only way a Logger should be obtained is through a LoggerProvider (at least, that's true for other signals, but not expressly forbidden by the logs bridge API), I've gone with an EventLoggerProvider containing a LoggerProvider, from which it gets its logger.
The event logger API suggests that the above is correct ("EventLoggers can be accessed with a EventLoggerProvider"), however the event logger SDK says that "The SDK MAY be implemented on top of the Logs Bridge API".