chore: improve exception logging across the SDK#7358
Conversation
| @@ -112,6 +119,10 @@ def build_logger(logger_name: str, source_type: Optional[str] = None) -> Any: | |||
| debug_on: bool = False | |||
| debug_level: Literal[1, 2] = 1 | |||
|
|
|||
| # Controls whether exc_info tracebacks are rendered in log output. | |||
| # Opt-in only: set AGNO_VERBOSE_EXCEPTIONS=true or call set_verbose_exceptions(True). | |||
| verbose_exceptions: bool = getenv("AGNO_VERBOSE_EXCEPTIONS", "false").lower() in ("true", "1", "yes") | |||
There was a problem hiding this comment.
AGNO_VERBOSE_EXCEPTIONS seems a bit ambiguous since it looks as if its scoped to exceptions only but we cater other logging outputs as well. Wdyt?
There was a problem hiding this comment.
yeah that's why this on slack thread to get more opinions on DX-
- added
exc_info=Trueinlog_warningbut its gated with a new env variableAGNO_VERBOSE_EXCEPTIONSwhich is by default False so it strips theexc_infofrom those logs, if turned itrue it keep it and prints the full traceback. - are we happy with this env variable approach or do we rather add some
debug_level=3for this? or some entirely new flag onagent/team/workflow(which does not sound elegant)
There was a problem hiding this comment.
We can also rename this to be more self explanatory something like AGNO_LOG_TRACEBACKS
| def set_verbose_exceptions(enabled: bool = True) -> None: | ||
| """Enable or disable full tracebacks in log output. | ||
|
|
||
| When enabled, ``log_exception`` and ``log_warning`` calls with |
There was a problem hiding this comment.
Shouldn't the log_exception's exc_info be ideally be True always?
There was a problem hiding this comment.
yes, updated so that it is not stripped in case of log_exception
sannya-singal
left a comment
sannya-singal
left a comment
There was a problem hiding this comment.
Thanks for following up on this, nice work! I just have few comments.
| ) | ||
| except Exception as e: | ||
| log_warning(f"Knowledge search failed: {e}") | ||
| log_warning("Knowledge search failed", exc_info=True) |
There was a problem hiding this comment.
Settings exc_info=True here is a bit misleading since it depends on the env var
| run_context=run_context, | ||
| ) | ||
| except Exception as e: | ||
| log_warning(f"Knowledge search failed: {e}") |
There was a problem hiding this comment.
Now for a simple log warning, user would get less information since we are removing {e}. Instead, we should keep the error statements as before with the option of expanding traceback if necessary
| @@ -32,6 +32,18 @@ def __init__(self, *args, source_type: Optional[str] = None, **kwargs): | |||
| super().__init__(*args, **kwargs) | |||
| self.source_type = source_type | |||
|
|
|||
| def emit(self, record: logging.LogRecord) -> None: | |||
There was a problem hiding this comment.
The emit() override is fine for gating tracebacks behind the env var, but we should keep {e} in the warning messages. Right now the default mode gives users less info than before — e.g. "Knowledge search failed" vs "Knowledge search failed: ConnectionRefused(...)".
Can we do both?
log_warning(f"Knowledge search failed: {e}", exc_info=True)
3 participants
Summary
log_error(f"...{str(e)}")withlog_exception("...")inexceptblocks across the SDK — uses the idiomatic Python pattern for logging errors with tracebacks.log_warning(f"...{str(e)}")withlog_warning("...", exc_info=True)in except blocks — stops swallowing exception context.AGNO_VERBOSE_EXCEPTIONSenv var (opt-in) to control traceback visibility — off by default (clean logs), full tracebacks when set to true.set_verbose_exceptions()andconfigure_agno_logging(enable_verbose_exceptions=...)for programmatic control.Type of change
Checklist
./scripts/format.shand./scripts/validate.sh)Duplicate and AI-Generated PR Check
Additional Notes
Add any important context (deployment instructions, screenshots, security considerations, etc.)