Skip to content

refactor(logging): quiet default observability in jido#245

Closed
nshkrdotcom wants to merge 4 commits intoagentjido:mainfrom
nshkrdotcom:log-refactor-phase-1
Closed

refactor(logging): quiet default observability in jido#245
nshkrdotcom wants to merge 4 commits intoagentjido:mainfrom
nshkrdotcom:log-refactor-phase-1

Conversation

@nshkrdotcom
Copy link
Copy Markdown
Contributor

Summary

Context

Refs #244

Testing

  • existing branch state

Copy link
Copy Markdown
Contributor Author

Companion upstream PRs are now open for the library-side portions of this logging cleanup:

Both reference #244 and this Jido PR for the broader ecosystem context.

A note on interdependencies: the Jido work in this branch was validated locally against those jido_action / jido_signal branches via path deps so the cross-repo boundary could be checked holistically. The committed state of this PR, however, still keeps the published Hex deps in place. The temporary local path-dep switch was left uncommitted on purpose so https://github.com/agentjido/jido/pull/245 stays clean and reviewable while the upstream library PRs are discussed and merged.

The net effect is:

  • jido_action owns the human log-line contract for action execution
  • jido_signal owns its runtime/log-helper cleanup and logger-adapter payload safety
  • jido owns config derivation and runtime telemetry behavior at the downstream boundary

Tagging @mikehostetler for visibility.

Copy link
Copy Markdown
Contributor Author

@mikehostetler Compatibility follow-up pushed on this branch.

This restores the historical top-level logging defaults while keeping the boundary cleanup from this PR.

What changed:

  • Jido.Config.Defaults.telemetry_log_level/0 is back to :info.
  • Jido.Config.Defaults.observe_log_level/0 is back to :info.
  • Tests still assert the jido boundary it actually owns: derived exec opts and telemetry behavior, not downstream human log strings.

How to opt in to the quieter runtime behavior introduced by this refactor:

  • config :jido, :telemetry, log_level: :warning
  • config :jido, :observability, log_level: :warning
  • keep action args suppressed via config :jido, :telemetry, log_args: :keys_only or :none
  • instance-level verbose/full behavior still works through Jido.Debug

Why this follow-up was needed:

  • The earlier pass changed the top-level defaults to :warning, which is a user-visible runtime behavior change.
  • Restoring the :info baseline keeps the logging cleanup non-breaking while preserving the new config-driven quiet path.

Stack note:

  • I also restacked jido-test-cleanup-phase-2 onto the updated log-refactor-phase-1 head and force-pushed that branch, so the test-cleanup stack now sits on top of the compatibility-restored logging branch.

Copy link
Copy Markdown
Contributor

@ccarvalho-eng ccarvalho-eng left a comment

Choose a reason for hiding this comment

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

This is a good step. Moving away from eager interpolation in debug logs is exactly the right habit for a library, and safe_inspect as a centralized policy makes sense.

One thing worth being explicit about: Jido.Log in core solves the problem for this repo, but it does not solve it for the ecosystem. The value here is the convention, not the module. Other jido_* repos should copy the shape, not take a dependency on core just to share a logger wrapper — that would be the wrong coupling. Make sure that is clear in the team so this doesn't quietly become an undocumented public API.

The duplicate error messages in cron_cancel.ex are a small but real issue — two different failure paths, same string. That will cost someone an hour at 3am.

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.

Requesting changes on the logging rollout shape.

I’m aligned with the goals here: quieter defaults, safer handling of inspect-hostile values, and lazy evaluation where it actually matters. I’m not aligned with introducing a repo-local logging facade like Jido.Log.

The ADR direction I want us to follow is: use base Logger directly, not a helper API layer. Please rework this PR back to direct Logger usage and keep any sanitizer / safe-inspect / redaction logic local to the observability boundary that actually owns it.

In other words: keep the behavior improvements, but remove the helper module and the abstraction spread that comes with it.

@nshkrdotcom
Copy link
Copy Markdown
Contributor Author

nshkrdotcom commented Apr 3, 2026

Yes, it's a smell to copy paste such a facade. My bad. I was hesitant to suggest an alternative:

A shared jido_logger introduces dependency challenges, but will be worth the stretch if 1) the shape can be made stable 2) it provides useful functionality beyond just centralizing logging code.

For now, I'll redo all four PR's per the spec. @mikehostetler May I ask where is an ADR for logging?

@nshkrdotcom
Copy link
Copy Markdown
Contributor Author

Superseded by #249 from . Closing this original logging PR without merge so review can continue on the direct-Logger replacement branch requested by Mike.

@nshkrdotcom nshkrdotcom closed this Apr 3, 2026
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