-
Notifications
You must be signed in to change notification settings - Fork 294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Logging Overhaul #2308
base: master
Are you sure you want to change the base?
Logging Overhaul #2308
Conversation
I solved the dependency issue, but I have a code generation issue now: In the macro code, in
Clearly, in this code I have not generated code. But I need to refer to the StreamSink, which should be generated ... Do you have an advice how to progress?
I am sure you have a better idea :) It is a bit cat-and-mouse: The logger code should be generated (actually integrated), but it needs the StreamSink to be generated to work ... As for solving the log dependency: Nevertheless, I want to leave this decision open until I see if |
Standard Rust would need |
The problem is, that there is no
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A deeper review, but mainly are nits :)
@@ -1,89 +1,147 @@ | |||
# Logging | |||
|
|||
Since I have seen some questions asking how logging can be implemented with a Flutter + Rust application, here are some examples. | |||
Flutter Rust Bridge comes with logging build in - but you can override it with your own logging framework of choice. | |||
More concrete, you can overwrite the log outputting function with the one(s) of your logging framework(s) of choice, which will automatically log statements from Rust as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo(fzyzcjy): optimize the doc
(you don't need to do anything - this note is for me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Note that I rewrote the docs after making the logging better to use. Feel free to change it anywhere you like :)
frb_rust/src/misc/logging.rs
Outdated
} | ||
|
||
/// initialize the logging system, including the rust logger | ||
static Logger init_logger( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like logger initialization indeed has multiple things. maybe we can separate them. for example:
- setup rust logging, such that whenever rust
log::info!("hi")
, it is forwarded to dart logginglog.info("hi")
. in this function, we do not configure any dart logging outputs / min levels etc. - another separate thing: configure a (default) Logger.root.level and Logger.root.onRecord.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the API, so that the configuration is simpler.
These settings need to happen at the same time.
The problem is this code:
pub fn initialize_log2dart(log_stream: crate::frb_generated::StreamSink<Log2DartLogRecord>, max_log_level: u16) {
log::set_boxed_logger(Box::new(FRBLogger {
stream_sink: log_stream,
}))
.map(|()| log::set_max_level(from_u16(max_log_level)))
.expect("initialize_log2dart is called only once!");
(...)
Because I move the FRBLogger
into log::
I cannot change the log level subsequentially anymore.
In between I had a version where one could set this and the logging function later in the program execution - but while I was struggling with lifetimes I thought that a user might never need to change these later in a program.
If it is just to make the function smaller and easier to read: Sure, we can split it up. But eventually it needs to be called at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But eventually it needs to be called at once.
By default yes. But users should be free to customize etc.
Suppose a simple example: User may like to have Rust log forward to Dart, but at the same time, he or she already setup the dart logging (e.g. his own Logger.root.level, his own Logger.root.onRecord, let's say he sends it to Sentry instead of printing for some reason). Then he should not call our 2nd function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot change the log level subsequentially anymore.
What about
pub fn another_function_that_freely_changes_log_level(){
log::set_max_level(TRACE)
}
seems we can change the level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will look into it. I forgot why I wasn't able to change it after the initialization ... maybe it was a temporary limitation during refactoring ...
But let's first refactor where the code will be generated to, do unit tests - and then I will enable setting the log level after initialization.
As for your example, a user could create another logger for Sentry
, that inherits from the FRB created root logger and change anything he likes there.
But in general, I think if a user changes something using this logging implementation, it should always apply to both sides, rust and dart. That way it is least surprising :)
frb_rust/src/misc/logging.rs
Outdated
} | ||
|
||
/// initialize the logging system, including the rust logger | ||
static Logger init_logger( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo(fzyzcjy): review this core file again after it is modified
Now I am thinking if it wouldn't have been better to implement logging from the other side around: Send Dart logs to rust. For me personally it would depend how to handle logging configuration best - I here I lack the experience. I mean, would it be better to have log outputs configured via rust (using For now I am undecided and would first gather some experience. I understood that in your product you forward the rust logs to dart as well, right? |
Hmm, if the user is "using Rust for e.g. computation-heavy work for the Flutter app", then I guess he or she will have already setup the Dart logging system before adding frb or Rust code, and thus "send rust logs to dart" would be a good default choice. My personal choice for my app is also send Rust log to Dart.
I guess it still needs macros (for complex scenarios at least) |
True.
Thanks for sharing.
True - at least the rust calls to be called by the Dart logging function (if Dart -> rust) need to be wired ... so the macro is a must. I am currently simplifying the usage a bit. If you like you can already have a look - but I will extend the macro with the same optional parameters as the dart function |
I finished the improvements - feel free to review this PR! |
Oh, I overlooked that you did already a review! Working through your comments now! |
I finished working through your review. Many thanks! Now only until tests are to-be-done. Sounds easy, but is probably not needed, as discussed before ... For now I will hold still until we resolved the above discussions. The bloating of the |
Great and you are welcome!
I guess we can add this option in doc only and mention "if you want it, create an issue and discuss your scenarios". I personally think this will not be very frequently needed, thus we can save time efforts not implementing it, but I can be totally wrong!
Totally agree (see my reply above) |
I looked into it, but it is not so easy to understand ... it would help, if you can give me a kick-start here: If you could implement a test for the default settings, meaning implementing a test that calls Based on that I can do all the other scenarios. As for asserting the test result, if it is too complicated to assert what has been written to the console we can set a custom log function that writes into a variable instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(just happen to see this, thus this isolated nit)
frb_codegen/assets/integration_template/shared/REPLACE_ME_RUST_CRATE_DIR/Cargo.toml.template
Outdated
Show resolved
Hide resolved
// let message = record.message; | ||
// println!("[{logger_name}] {message})"); | ||
// }) | ||
// ); | ||
|
||
#[frb(init)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: thinking about what an end user sees. feel free to discuss!
In main.dart
Nothing. There is no line related to logging setup.
In minimal.rs
i.e. what should be the code in minimal.rs, which can either by our default template, or by telling users to migrate to
choice 1
flutter_rust_bridge::setup_default_logging!();
#[frb(init)]
pub fn init_app() {
flutter_rust_bridge::setup_default_user_utils();
}
choice 2
flutter_rust_bridge::setup_default_user_utilities!();
(the name is explicitly a bit different to avoid making a macro and a function same name to confuse users. the current hacky proposed name looks not good... feel free to propose a better name!)
where setup_default_user_utils
's definition is, "call setup_default_logging + that fn init_app".
and we still allow customizations. For example, suppose one wants to remove logging, but keep others, then he or she can remove setup_default_user_utils, and copy-paste the fn initapp
or whatever he likes.
pros: less confusing to users (in choice 1, users may think "why logging is not user utils?")
pros: less verbose (in choice 1, user may think "oh so many lines")
cons: a bit less explicit. (but users can click that macro to see what is going on, so maybe acceptable)
I personally prefer choice 2, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with choice 2 as well :) probably there will be more supporting tools in future - and then it would get very (too) verbose. So hiding that all behind something like setup_default_(...)
sounds very good to me :)
I am quite confident, the I can rewrite the code so that the customizing can be done any time ... from setting the log level to a custom log function.
I think this would work from both sides - meaning one could call a Rust function set_custom_log_fn
from minimal.rs
or main.dart
. So my next question is not very relevant, but: We are assuming a user would do the logging configuration preferably from Dart, right?
Thus, for setting the log level one needs to use the Dart logging package's definition (which I find quite unusual ...), even when setting it from Rust. I could easily change this though, I was aiming for consistency.
As for the custom log function, if set from Dart it shall be Dart code, if set from Rust Rust code. I think we can change this as well, but that requires a bit more effort. But probably this is a good default.
So, to summarize, I think it would be best if:
- logging gets automatically initialized with defaults, triggered as part of RustLib.init()
- A user would disable this by not having the
log
feature active - thus, there is no visible code in neither
main.dart
, norminimal.rs
- issuing log statements works as usual in the respective language (
Logger().info('from Dart)
, log::info(from Rust
) - this should already work :) - Customizing the log level can be done from both, Dart or Rust code
- the generated code ends up in frb_generated.rs/dart and not minimal.rs
For (6.), I wasn't able to get it there - the StreamSink was never processed. I tried calling the macro in frb_rust/src/for_generated/boilerplate.rs
, frb_rust/src/misc/user_utils.rs
, etc. Probably you see right away what needs to be fixed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably there will be more supporting tools in future
Totally agree. Choice 1 will require user to migrate again at that time which is not optimal.
Agree 1-6! Except minor details:
A user would disable this by not having the log feature active
I guess by simply copy-paste content in setup_default_user_utilities and remove the one about logging.
Using feature for this, personally speaking, may be a little bit overkill, and a bit less flexible.
Customizing the log level can be done from both, Dart or Rust code
Yes, for simplicity (of our implementation), users can use Rust and Dart's native mechanism to set logging level respectively.
the generated code ends up in frb_generated.rs/dart and not minimal.rs
I will handle that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A user would disable this by not having the log feature active
I guess by simply copy-paste content in setup_default_user_utilities and remove the one about logging. Using feature for this, personally speaking, may be a little bit overkill, and a bit less flexible.
Yes, true, that would be a way as well. I was referring to the already existing feature flag 'log' - I used #[cfg(feature = "log")]
wherever it made sense :)
Customizing the log level can be done from both, Dart or Rust code
Yes, for simplicity (of our implementation), users can use Rust and Dart's native mechanism to set logging level respectively.
Oh yes, we can just use the usual functions for setting the log level - will do that.
the generated code ends up in frb_generated.rs/dart and not minimal.rs
I will handle that
Ah, great , thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That flag was there because I wanted frb_rust to have minimal dependency if a user wants it. It accidentally also controls that existing behavior...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea :) Ah, ok. If this flag is needed always for the existing behavior (mandatory parts?) I would suggest to remove it from these and repurpose it for this logging implementation. Probably users have used it for that. I actually assume that probably nobody deactivated it ... as this is not so straight-forward in Rust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, maybe we can add a few lines in codegen to sanity check, if we find the string setup_default_user_utils
, maybe we just log::warn!("Please migrate to new default user utils, see some_url_link_here for details")
in codegen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be great!
Changes
Please list issues fixed by this PR here, using format "Fixes #the-issue-number".
Checklist
./frb_internal precommit --mode slow
(orfast
) is run (it internal runs code generator, does auto formatting, etc)../website
folder) are updated.Remark for PR creator
./frb_internal --help
shows utilities for development.