Skip to content

feat: support logging of exceptions#277

Open
IzaakGough wants to merge 16 commits into
mainfrom
@invertase/feat-support-exception-logging
Open

feat: support logging of exceptions#277
IzaakGough wants to merge 16 commits into
mainfrom
@invertase/feat-support-exception-logging

Conversation

@IzaakGough

@IzaakGough IzaakGough commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

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

  • Updated tests/test_logger.py to cover exception instances, exception types from sys.exc_info()[0], cyclic/self-referential payloads, non-JSON-safe values, and top-level stack_trace behavior for ERROR logs.
  • Ran python3 -m pytest -q tests/test_logger.py

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/firebase_functions/logger.py Outdated
Comment thread src/firebase_functions/logger.py Outdated
@IzaakGough IzaakGough marked this pull request as ready for review June 3, 2026 10:04
@IzaakGough IzaakGough requested a review from a team June 3, 2026 10:04
@cabljac cabljac self-requested a review June 8, 2026 09:35
@cabljac

cabljac commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

logging library doesn't support Exceptions

2 participants