-
Notifications
You must be signed in to change notification settings - Fork 820
Add logger exception support for logs API/SDK #4908
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?
Changes from all commits
9c21864
26ac99b
d226158
6581718
0f2bcf0
a42e569
b02a455
4003ef3
bcafcc1
9ca005d
a4c169c
bf10796
9689d30
b1d7f0d
bd6ba38
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 |
|---|---|---|
|
|
@@ -516,6 +516,78 @@ def force_flush(self, timeout_millis: int = 30000) -> bool: | |
| ) | ||
|
|
||
|
|
||
| def _get_exception_attributes( | ||
| exception: BaseException, | ||
| ) -> dict[str, AnyValue]: | ||
| stacktrace = "".join( | ||
| traceback.format_exception( | ||
| type(exception), value=exception, tb=exception.__traceback__ | ||
| ) | ||
| ) | ||
| module = type(exception).__module__ | ||
| qualname = type(exception).__qualname__ | ||
| exception_type = ( | ||
| f"{module}.{qualname}" if module and module != "builtins" else qualname | ||
| ) | ||
| return { | ||
| exception_attributes.EXCEPTION_TYPE: exception_type, | ||
| exception_attributes.EXCEPTION_MESSAGE: str(exception), | ||
| exception_attributes.EXCEPTION_STACKTRACE: stacktrace, | ||
| } | ||
|
|
||
|
|
||
| def _get_attributes_with_exception( | ||
| attributes: _ExtendedAttributes | None, | ||
| exception: BaseException | None, | ||
| ) -> _ExtendedAttributes | None: | ||
| if exception is None: | ||
| return attributes | ||
|
|
||
| exception_attributes_map = _get_exception_attributes(exception) | ||
| if attributes is None: | ||
| attributes_map: _ExtendedAttributes = {} | ||
| else: | ||
| attributes_map = attributes | ||
|
|
||
| if isinstance(attributes_map, BoundedAttributes): | ||
| bounded_attributes = attributes_map | ||
| merged = BoundedAttributes( | ||
| maxlen=bounded_attributes.maxlen, | ||
| attributes=bounded_attributes, | ||
| immutable=False, | ||
| max_value_len=bounded_attributes.max_value_len, | ||
| extended_attributes=bounded_attributes._extended_attributes, # pylint: disable=protected-access | ||
| ) | ||
| merged.dropped = bounded_attributes.dropped | ||
| for key, value in exception_attributes_map.items(): | ||
| if key not in merged: | ||
| merged[key] = value | ||
| return merged | ||
|
|
||
| return exception_attributes_map | dict(attributes_map.items()) | ||
|
|
||
|
|
||
| def _copy_log_record( | ||
| record: LogRecord, | ||
| attributes: _ExtendedAttributes | None, | ||
| ) -> LogRecord: | ||
| copied_record = LogRecord( | ||
| timestamp=record.timestamp, | ||
| observed_timestamp=record.observed_timestamp, | ||
| context=record.context, | ||
| severity_text=record.severity_text, | ||
| severity_number=record.severity_number, | ||
| body=record.body, | ||
| attributes=attributes, | ||
| event_name=record.event_name, | ||
| exception=getattr(record, "exception", None), | ||
| ) | ||
| copied_record.trace_id = record.trace_id | ||
| copied_record.span_id = record.span_id | ||
| copied_record.trace_flags = record.trace_flags | ||
| return copied_record | ||
|
|
||
|
|
||
| class LoggingHandler(logging.Handler): | ||
| """A handler class which writes logging records, in OTLP format, to | ||
| a network destination or file. Supports signals from the `logging` module. | ||
|
|
@@ -671,32 +743,54 @@ def emit( | |
| body: AnyValue | None = None, | ||
| attributes: _ExtendedAttributes | None = None, | ||
| event_name: str | None = None, | ||
| exception: BaseException | None = None, | ||
| ) -> None: | ||
| """Emits the :class:`ReadWriteLogRecord` by setting instrumentation scope | ||
| and forwarding to the processor. | ||
| """ | ||
| # If a record is provided, use it directly | ||
| if record is not None: | ||
| record_exception = exception or getattr(record, "exception", None) | ||
|
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. Personally, I'm not a huge fan of modifying attributes of an already created log record. For example, if the attributes in the created log record are immutable, the |
||
| if record_exception is None and isinstance( | ||
| record, ReadWriteLogRecord | ||
| ): | ||
| record_exception = getattr( | ||
| record.log_record, "exception", None | ||
| ) | ||
| if not isinstance(record, ReadWriteLogRecord): | ||
|
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. Might not be relevant to this PR but do we know what this instance check is here? Doesn't
Author
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.
|
||
| if record_exception is not None: | ||
| record = _copy_log_record( | ||
| record, | ||
| _get_attributes_with_exception( | ||
| record.attributes, record_exception | ||
| ), | ||
| ) | ||
| # pylint:disable=protected-access | ||
| writable_record = ReadWriteLogRecord._from_api_log_record( | ||
| record=record, | ||
| resource=self._resource, | ||
| instrumentation_scope=self._instrumentation_scope, | ||
| ) | ||
| else: | ||
| record.log_record.attributes = _get_attributes_with_exception( | ||
| record.log_record.attributes, record_exception | ||
| ) | ||
| writable_record = record | ||
| else: | ||
| # Create a record from individual parameters | ||
| log_record_attributes = _get_attributes_with_exception( | ||
| attributes, exception | ||
| ) | ||
| log_record = LogRecord( | ||
| timestamp=timestamp, | ||
| observed_timestamp=observed_timestamp, | ||
| context=context, | ||
| severity_number=severity_number, | ||
| severity_text=severity_text, | ||
| body=body, | ||
| attributes=attributes, | ||
| attributes=log_record_attributes, | ||
| event_name=event_name, | ||
| exception=exception, | ||
| ) | ||
| # pylint:disable=protected-access | ||
| writable_record = ReadWriteLogRecord._from_api_log_record( | ||
|
|
||
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.
nit: When
recordis provided alongside anexceptionkwarg, the exception is silently dropped here (only forwarded in theelsebranch). The SDK'sLogger.emit()explicitly handles both (exception or getattr(record, "exception", None)), so there's a subtle inconsistency between the proxy and SDK paths.Not a contract violation (the overloads define them as separate call forms), but it might be worth either forwarding
exceptionhere too (self._logger.emit(record, exception=exception)) or adding a comment explaining why it's intentionally omitted.