Skip to content

refactor(logging): localize core observability logging with direct Logger#249

Open
nshkrdotcom wants to merge 5 commits intoagentjido:mainfrom
nshkrdotcom:logger-refactor-cleanup
Open

refactor(logging): localize core observability logging with direct Logger#249
nshkrdotcom wants to merge 5 commits intoagentjido:mainfrom
nshkrdotcom:logger-refactor-cleanup

Conversation

@nshkrdotcom
Copy link
Copy Markdown
Contributor

Summary

  • rework the logging cleanup onto Mike Hostetler's requested shape for jido core
  • remove the helper logging api layer and use base Logger directly
  • keep safe inspect and log-threshold behavior local to the observability and telemetry boundaries that own it
  • preserve historical top-level defaults while keeping quieter behavior opt-in via config

Supersedes

Mike-Requested Rework

  • remove Jido.Observe.Log / helper logging api spread
  • use direct Logger calls
  • keep lazy logging only where it actually matters
  • keep sanitization/redaction local to core observability ownership boundaries
  • preserve compatibility by default

What This PR Keeps

  • quieter and cheaper logging on hot paths
  • safer handling of inspect-hostile values in telemetry and runtime logging
  • compatibility-first default config behavior in core

Additional Work

Implementation

  • no additional runtime feature work beyond the direct-Logger/core-boundary rework itself

Tests

  • includes test-only follow-up hardening that landed on this branch after the logging rewrite:
    • remove the scheduler warning caused by the invalid receive ... after :infinity test construct
    • harden one Agent retry test that compared elapsed wall-clock time
    • harden one AgentServer scheduled-message test that depended on an overly tight timing budget

Impact

  • top-level jido logging defaults remain at the historical :info baseline
  • test suite stability improves without changing runtime contracts
  • the branch now matches the direct-Logger shape requested in review

Risks

  • low runtime risk because the additional work in this branch outside the logging rewrite is test-only
  • reviewers should still note that this replacement branch includes both the logging rework and test hardening, even though the latter is non-runtime

Testing

  • mix test
  • mix q

@nshkrdotcom nshkrdotcom force-pushed the logger-refactor-cleanup branch from 2cb6299 to 7e0e853 Compare April 3, 2026 23:03
end)
else
Logger.warning("Jido.Signal.Dispatch not available, skipping error signal emit")
Logger.warning(fn -> "Jido.Signal.Dispatch not available, skipping error signal emit" end)
Copy link
Copy Markdown
Contributor

@ccarvalho-eng ccarvalho-eng Apr 4, 2026

Choose a reason for hiding this comment

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

It's unclear the benefits we'd have for using a function inside warning/1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's for lazy evaluation, just a performance tweak

Copy link
Copy Markdown
Contributor

@mikehostetler mikehostetler left a comment

Choose a reason for hiding this comment

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

Two blocking findings from review:

  1. lib/jido/observe.ex removes the public Jido.Observe.log/3 facade and the shipped Jido.Observe.Log module. That is a breaking API change for downstream callers, not just an internal refactor, so existing users will start hitting UndefinedFunctionError after upgrade unless this is kept as a compatibility shim or deprecated first.

  2. lib/jido/observe/config.ex still resolves observe_log_level/1, and Jido.Debug still writes :observe_log_level overrides, but after deleting Jido.Observe.Log there is no runtime consumer left for that setting. That silently turns config :jido, :observability, log_level: ... and the related debug override path into dead config, which is a behavior regression even if callers never used the removed facade.

Introduced Jido.Observe.Log to centralize threshold-based logging logic.
Added Jido.Observe.log/3 as a facade to maintain compatibility with the
existing observability API. The implementation respects both the global
Jido observability configuration and per-instance overrides via Jido.Debug.
Includes comprehensive test coverage for threshold enforcement and instance
metadata handling.
@nshkrdotcom
Copy link
Copy Markdown
Contributor Author

Ready for review. Went with compatibility shim rather than tagging as @deprecated.

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.

3 participants