Implement adr-9 logging; implemented via tracing. #6748

mhammond · 2025-05-06T20:12:11Z

No changes required for mobile consumers, it's all abstracted in rust_log_forwarder

Summary of changes:

Existing error-support crate re-exports log, info, warn, trace etc. All existing components are changed so that instead of using log:: directly we use the re-exports from error-support.
error-support also grows a testing feature to expose a couple of helpers for tests.
new crate tracing-support which also re-exports log, info, warn, trace etc and also has a testing feature which re-exports the same test functions.
error-support also grows new features to control whether it re-exports the log calls from the log and env_logger crates, or from tracing-support.

The intent here is that in app-services we will use the feature which uses tracing (because we also have rust_log_forwarder setup to support that), but initially on desktop we will enable the feature to continue using log while we work on setting up an appropriate tracing subscriber there.

There's also a feature to control how error reporting is done - by default it is done like has always been done on mobile, but is setup so that on Desktop it will use the tracing facility. This means we can eventually end up using a single tracing subscriber for both error reports and logs (and also any other event types we invent in the future).

mhammond · 2025-05-13T14:36:42Z

Hey @bendk, sorry about this 😬 Let's talk about it later today.

bendk

Overall, I love these changes.

There were a couple pieces of leftover debug code that should be removed. Other than that, the only "blocker" for me is the question of how we filter for app-services events. Is a list of target strings the best way? The answer may be yes, but I just wanted to discuss it some.

bendk · 2025-05-14T17:46:50Z

components/places/src/api/places_api.rs

+        println!("XXXXX - hello println");
+        eprintln!("XXXXX - hello eprintln");
+        error!("oh no places");
+        //panic!("at the disco!!!");


This is hilarious, but probably should be deleted.

bendk · 2025-05-14T18:58:19Z

components/support/tracing/src/layer.rs

+        // println!(
+        //     "TRACING EVENT {target} - {prefix} - {}",
+        //     SINKS_BY_TARGET.read().len()
+        // );


Nit: another piece of leftover code.

bendk · 2025-05-14T19:04:00Z

components/support/rust-log-forwarder/src/lib.rs

+
+// The "targets" (in practice, crate names) which are hooked up to the `tracing` crate for logging.
+// We should improve this, or better still, just kill this crate entirely - but it is a natural consequence
+// of using `tracing` that each target must be explicitly listened for *somewhere*.


Can you expand on this one a bit. Killing this crate sounds great, but how would that work in practice?

Also, what do you mean by "each target must be explicitly listened for somewhere."? When I first read that, I added "..or else something would break" in my head. Now that I re-read it, it seems like we would lose the logs. I think you framed this on our call as "there's no way to globally listen for all events", which seems clearer to me.

bendk · 2025-05-14T19:05:49Z

components/support/rust-log-forwarder/src/lib.rs

-pub use foreign_logger::{AppServicesLogger, Level, Record};
+// ideally we'd use tracing::Level as an external type, but that would cause a breaking change
+// for mobile, so we clone it.
+// (it's a shame uniffi can't re-export types!)


Cloning seems okay to me, but I don't understand what's stopping us from using this as an external type. Can you link to an issue?

It can work as an "external type", but in that scenario it means that the existing rust-log-support in the mobile platforms needs to change - eg, we'd need a PR for Swift and Android which subtly changes the dependencies so that the new crate tracing_support is referenced. IOW, this is just a work-around to avoid the breaking-change dance.

The second comment was a reflection on this - it's kinda a shame we can't have uniffi pretend Level was directly exported by rust-log-forwarder, but I've no idea how that would work - maybe I should remove that comment?

wdyt?

Ahh, I see what you mean now.

The "re-export types" feature seems doable to me. Maybe it already works on some languages. For example python imports the type name, that might be enough. We probably should be adding the name to __all__ as well though.

bendk · 2025-05-14T19:56:37Z

components/support/rust-log-forwarder/src/lib.rs

+    // Set up a tracing subscriber for crates which use tracing and forward to the foreign log forwarder.
+    for target in TRACING_TARGETS {
+        tracing_support::register_event_sink_arc(target, (*level).into(), sink.clone())
+    }


I think this should work fine, but I want to try to figure out how to wire this all up differently -- mostly just to think about the possibilities here and understand the requirements better.

Could we have a tracing field that distinguished app-services events from other events and have this code subscribe to all those events at once? For example, the error_support macros could set a boolean field named app_services and the tracing_support code could decide if an event should be forwarded to the event sink by looking for the presence of that field rather than filtering on the target.

I think the problem here is that we'd also need to filter on the target? ie, we'd need some way of getting all targets and then filtering after the target matches, which I'm not sure if possible, and if it was, would leave us in the same place we are in the log crate wrt performance impact of intercepting and formatting logs we don't ultimately care about.

I was thinking that we could replace this code in tracing_support and then we get access to all the events, regardless of target.

However, looking through that code again makes me see some bigger-picture issues. By using target as the basis of the filter, you could easily extend that to having different levels per-target and having that filtering happen before anything gets pushed across the FFI. That makes me now want to change anything. I feel like there might be some changes down the road, especially as we try to make this work for both error reporting and logging, but the current code seems like the best place to start.

bendk

Thanks for talking through that filtering question with me. I think this one is ready to merge once those printlns are removed from places_api.rs

bendk · 2025-05-15T16:32:40Z

components/support/rust-log-forwarder/src/lib.rs

-pub use foreign_logger::{AppServicesLogger, Level, Record};
+// ideally we'd use tracing::Level as an external type, but that would cause a breaking change
+// for mobile, so we clone it.
+// (it's a shame uniffi can't re-export types!)


Ahh, I see what you mean now.

The "re-export types" feature seems doable to me. Maybe it already works on some languages. For example python imports the type name, that might be enough. We probably should be adding the name to __all__ as well though.

bendk · 2025-05-15T16:42:21Z

components/support/rust-log-forwarder/src/lib.rs

+    // Set up a tracing subscriber for crates which use tracing and forward to the foreign log forwarder.
+    for target in TRACING_TARGETS {
+        tracing_support::register_event_sink_arc(target, (*level).into(), sink.clone())
+    }


I was thinking that we could replace this code in tracing_support and then we get access to all the events, regardless of target.

However, looking through that code again makes me see some bigger-picture issues. By using target as the basis of the filter, you could easily extend that to having different levels per-target and having that filtering happen before anything gets pushed across the FFI. That makes me now want to change anything. I feel like there might be some changes down the road, especially as we try to make this work for both error reporting and logging, but the current code seems like the best place to start.

mhammond · 2025-05-15T18:08:13Z

I was thinking that we could replace this code in tracing_support and then we get access to all the events, regardless of target.

huh, yeah, maybe that could work! I do agree we shouldn't over-rotate now because I doubt many of these limitations are going to be a problem in practice. Eg, even today, all logs from sql-support don't indicate what component they originated in, but in practice this isn't a problem we've noticed because there's enough context to guess that. So let's just see what actually hurts us once this is in place.

bendk · 2025-05-15T18:12:23Z

Weird, I thought I already approved. In any case, it's looking great to me.

mhammond · 2025-05-19T00:43:15Z

/taskcluster ci full

…r logging. Summary of changes: * Existing `error-support` crate re-exports `log`, `info`, `warn`, `trace` etc. All existing components are changed so that instead of using `log::` directly we use the re-exports from `error-support`. * `error-support` also grows a `testing` feature to expose a couple of helpers for tests. * new crate `tracing-support` which also re-exports `log`, `info`, `warn`, `trace` etc and also has a `testing` feature which re-exports the same test functions. * `error-support` also grows new features to control whether it re-exports the log calls from the `log` and `env_logger` crates, or from `tracing-support`. The intent here is that in app-services we will use the feature which uses `tracing` (because we also have `rust_log_forwarder` setup to support that, but initially on desktop we will enable the feature to continue using `log` while we work on setting up an appropriate tracing subscriber there. There's also a feature to control how error reporting is done - by default it is done like has always been done on mobile, but is setup so that on Desktop it will use the tracing facility. This means we can eventually end up using a single tracing subscriber for both error reports and logs (and also any other event types we invent in the future).

mhammond force-pushed the log-to-tracing branch 7 times, most recently from 2cc1a10 to 232db75 Compare May 6, 2025 21:25

mhammond changed the title ~~Move autofill to tracing crate, add new support/tracing module for te…~~ Implement adr-9 logging; implemented via tracing. May 7, 2025

mhammond force-pushed the log-to-tracing branch 12 times, most recently from 44d3fcf to 4cf0c93 Compare May 12, 2025 14:56

mhammond requested a review from bendk May 13, 2025 14:36

mhammond marked this pull request as ready for review May 13, 2025 14:36

bendk requested changes May 14, 2025

View reviewed changes

bendk approved these changes May 15, 2025

View reviewed changes

mhammond force-pushed the log-to-tracing branch from 4cf0c93 to c2bc483 Compare May 15, 2025 18:09

mhammond requested a review from bendk May 15, 2025 18:09

bendk approved these changes May 15, 2025

View reviewed changes

mhammond force-pushed the log-to-tracing branch from c2bc483 to 27efc53 Compare May 19, 2025 00:38

mhammond force-pushed the log-to-tracing branch from 27efc53 to 183c2f2 Compare May 20, 2025 00:47

mhammond force-pushed the log-to-tracing branch from 183c2f2 to a0b465b Compare May 21, 2025 00:37

mhammond added this pull request to the merge queue May 21, 2025

Merged via the queue into mozilla:main with commit 75a87f0 May 21, 2025
16 checks passed

mhammond deleted the log-to-tracing branch May 21, 2025 02:18

Implement adr-9 logging; implemented via tracing. #6748

Implement adr-9 logging; implemented via tracing. #6748

Uh oh!

Conversation

mhammond commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhammond commented May 13, 2025

Uh oh!

bendk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhammond commented May 15, 2025

Uh oh!

bendk commented May 15, 2025

Uh oh!

mhammond commented May 19, 2025

Uh oh!

Uh oh!

Uh oh!

mhammond commented May 6, 2025 •

edited

Loading