engineering: osutils: move generic log wrappers (MultiLogger, LogFilter) to osutils::logging - #751
Merged
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves two generic log-facade wrappers (MultiLogger, LogFilter<T>) out of the trident binary crate into a new osutils::logging module, aligning the implementation with the workspace’s layering goals (generic, reusable building blocks live in lower-level crates).
Changes:
- Introduces
crates/osutils/src/logging/{mod.rs,filter.rs,multilog.rs}and wires it intoosutilsvialib.rs. - Updates
tridentcall sites/imports to useosutils::logging::{filter, multilog}and removes the oldtridentre-exports/submodules. - Enables the
logcrate’sstdfeature forosutilsto supportset_boxed_loggerusage.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/trident/src/main.rs | Switches imports to osutils::logging and removes LogFilter/MultiLogger from trident re-exports usage. |
| crates/trident/src/logging/mod.rs | Removes filter/multilog submodule declarations after moving them to osutils. |
| crates/trident/src/logging/logstream.rs | Updates LogFilter import to the new osutils::logging::filter::LogFilter path. |
| crates/trident/src/lib.rs | Stops re-exporting LogFilter and MultiLogger from trident. |
| crates/osutils/src/logging/multilog.rs | Adds module-level docs for the moved MultiLogger. |
| crates/osutils/src/logging/mod.rs | Introduces the new osutils::logging module and exports filter/multilog. |
| crates/osutils/src/logging/filter.rs | Adds module-level docs and promotes into_inner to a non-test public accessor. |
| crates/osutils/src/lib.rs | Registers the new logging module in osutils. |
| crates/osutils/Cargo.toml | Enables log’s std feature for osutils. |
Suppressed comments (1)
crates/osutils/src/logging/multilog.rs:8
- The
MultiLoggerdocs say it “applies a global max level … before dispatching”. In the implementation, the max level is enforced via thelogfacade’s global max level wheninit()installs the logger, not byMultiLoggeritself when used directly. Clarifying this avoids over-promising behavior to callers who might useMultiLoggerwithout callinginit().
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
|
/azp run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
bfjelds (bfjelds)
approved these changes
Aug 25, 2026
Paco Huelsz (frhuelsz)
changed the title
Contributor
Author
|
/AzurePipelines run [GITHUB]-trident-pr |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
…s::logging
multilog.rs and filter.rs are strictly generic wrappers around the `log`
facade with no Trident coupling (no Trident types, no I/O, no policy), so per
the crate-layering guidelines they belong in osutils, not the trident binary.
- Move logging/{multilog,filter}.rs to crates/osutils/src/logging/ and register
the new `osutils::logging` module.
- Enable the `log` crate's `std` feature for osutils (needed by
MultiLogger::init's set_boxed_logger/set_max_level, previously satisfied only
transitively in the trident graph).
- Promote LogFilter::into_inner to a normal public accessor (it was #[cfg(test)],
which no longer works now that its only test caller lives in a different
crate) and drop the now-unnecessary #[allow(dead_code)] on with_max_level.
- Re-export LogFilter/MultiLogger from trident's lib.rs via osutils so
`trident::{LogFilter, MultiLogger}` and main.rs are unchanged; point
logstream.rs at osutils::logging::filter::LogFilter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Paco Huelsz (frhuelsz)
force-pushed
the
user/frhuelsz/osutils-logging
branch
from
August 26, 2026 19:42
9ce3176 to
bd92cfc
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/osutils/src/logging/mod.rs:1
- The module-level docs say these wrappers are "dependency-free", but the module necessarily depends on the
logcrate (and can also pull instd). Rewording to emphasize "no Trident-specific dependencies" / "thin wrappers around thelogfacade" would be more accurate.
//! Generic, dependency-free wrappers around the [`log`] facade.
bfjelds (bfjelds)
approved these changes
Aug 26, 2026
bfjelds (bfjelds)
added a commit
that referenced
this pull request
Aug 26, 2026
PR #751 (merged into main) moved MultiLogger/LogFilter from trident into a new osutils::logging/ directory module, while this branch had independently added its own osutils::logging.rs (single file) containing FilteredLogger - same module path, different shapes, causing an E0761 module-ambiguity build error after rebasing onto post-#751 main. Resolved by treating #751's shared LogFilter as the canonical implementation (it already supports everything FilteredLogger did, via its existing with_global_filter builder, matching the exact pattern trident/src/main.rs already uses for its own noisy-target suppression) and porting trident-acl-agent's two call sites onto it, rather than keeping a parallel duplicate wrapper type: - Deleted crates/osutils/src/logging.rs (FilteredLogger). - crates/trident-acl-agent/src/main.rs: added a small build_logger() helper that chains LogFilter::with_global_filter() over NETWORK_LOG_TARGETS, replacing FilteredLogger::new(). max_level is now computed inline (verbosity.max(network_verbosity)) since LogFilter has no equivalent to FilteredLogger::max_level(). Verified: cargo build/test/clippy/fmt clean for osutils, trident-acl-agent, and trident (386+148+158 tests pass).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Move the two strictly-generic
log-facade wrappers out of thetridentbinary and into a newosutils::loggingmodule:crates/trident/src/logging/multilog.rs→crates/osutils/src/logging/multilog.rs(MultiLogger)crates/trident/src/logging/filter.rs→crates/osutils/src/logging/filter.rs(LogFilter<T>)Why
Both are thin, dependency-free wrappers around the
logfacade — no Trident types, no I/O, no policy (a non-Trident project could use them verbatim). Per the crate-layering guidelines ("Is it a thin wrapper around a system tool/syscall…? →osutils"; lower layers own reusable, behavior-light building blocks), they belong inosutils, not the top-level binary.Changes
osutils::loggingmodule (mod.rs+ registered inosutils/src/lib.rs), with module-level docs on each wrapper describing purpose/capabilities.logcrate'sstdfeature forosutils—MultiLogger::initusesset_boxed_logger/set_max_level, which arestd-gated and were previously only satisfied transitively in thetridentdependency graph.LogFilter::into_innerpromoted from#[cfg(test)]to a normal public accessor (its only test caller now lives in a different crate, whereosutilsbuilds without test cfg); dropped the now-unnecessary#[allow(dead_code)]onwith_max_level.osutils::logging::{filter,multilog}—main.rsandlogging/logstream.rsupdated; no re-export left intrident/src/lib.rs.Validation
cargo check/clippyclean for bothosutilsandtrident(no unused imports).cargo test -p osutils logging::— the 4 moved unit tests pass.cargo test -p trident --lib logging::— all 20 logging tests pass (including the cross-crateLogFilter::into_innerpath inlogstream).Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com