feat: support logging of exceptions - #277
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request introduces exception logging capabilities to the logger, including a new exception function to log active stack traces and utility functions to safely serialize exceptions and handle circular references in exception arguments. Comprehensive tests have been added to verify these changes. The review feedback suggests improving type safety by changing the type of the refs parameter from set[_typing.Any] to set[int] in both _exception_from_args and _remove_circular, as it is used to track object IDs.
Sorry, something went wrong.
|
My main concern here is divergence from what JS does. I think JS puts the stack trace into the message field. It has no separate field or exception dict equivalent. Generally we need to keep the SDKs aligned Can we check if exceptions logged via this new approach show up in the error reporting console? |
Sorry, something went wrong.
|
Dug into the Error Reporting docs: jsonPayload is evaluated stack_trace -> exception -> message, so the dedicated field is the highest-priority path and traceback.format_exception gives the format it parses. So I'm now happy with the dedicated field and don't think we need to align with JS further. Could we add a code comment where we set stack_trace noting why we diverge from JS (it puts the trace in message; we use the dedicated field because Error Reporting checks it first)? Just so nobody realigns it later and regresses it. |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Fixes #172
Summary
This updates firebase_functions.logger so ERROR logs can safely include exception values and other non-JSON-safe payloads without failing serialization, while aligning the emitted traceback shape with the JS SDK. Stack traces now surface at top-level stack_trace, and the Python-only structured exception surface introduced earlier in the PR is removed.
Problem/Root Cause
The original logger could fail when error metadata included exception-related values that were not directly JSON serializable, including the documented sys.exc_info()[0] pattern and exception arguments containing opaque objects, bad repr() implementations, or cyclic data.
The initial fix in this PR added a Python-specific structured exception payload under error and a new logger.exception(...) helper. Reviewer feedback called out that this diverged from the JS SDK's logging shape, especially around traceback placement and the public API surface.
Solution/Changes
The logger now applies the defensive serialization work generically, keeping circular-reference handling and safe repr coercion for arbitrary metadata values instead of treating exceptions as a special nested schema.
For ERROR severity, the logger inspects the provided arguments and active exception state, then emits any available traceback at top-level stack_trace. Exception values passed via error= remain ordinary serialized metadata, so opaque or non-JSON-safe exception arguments no longer crash logging, but logs no longer emit nested-only traces or a Python-only logger.exception(...) helper.
The test coverage was updated to match the final behavior, including exception instances, sys.exc_info()[0], self-referential and cyclic payloads, non-JSON-safe values, and the top-level stack trace shape.
Testing