Skip to content
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

Allow to configure a custom logging backend #134

Closed
marcmo opened this issue Jan 28, 2021 · 8 comments
Closed

Allow to configure a custom logging backend #134

marcmo opened this issue Jan 28, 2021 · 8 comments
Labels
enhancement New feature or request nj-derive

Comments

@marcmo
Copy link
Contributor

marcmo commented Jan 28, 2021

It's currently not possible to configure a custom logging backend. What is needed is a way to use a custom logger along with some guidance of where to initialize it (since we do not have a main where this is done for applications.

@nicholastmosher
Copy link
Contributor

Thanks for the submission @marcmo! I'm going to write down some notes and observations here :)

Right now it seems that module "initialization" is somewhat static. We can see that the logging implementation is registered here in init_module, which has the #[ctor] attribute. I believe that this causes the init_module code to be loaded at least before any other module code, my guess is that it happens when the dynamic loader loads the shared object file.

Probably something we can think about doing is adding some sort of hook where module authors can define their own initialization function and init_module can call out to it. From a user-interface perspective, it would probably be possible to create an attribute such as #[node_bindgen(init)] that can be used to indicate that a particular function should be used for initialization.

#[node_bindgen(init)]
fn init_my_module() {
    // Logging init or other global setup
}

Implementation-wise, I'm imagining that we could probably use some sort of static that holds a reference to a Fn() -> () (or whatever we decide the init function signature should be) which is initialized to some empty or default init function (perhaps controlled by a feature flag), and can be overridden if a #[node_bindgen(init)] attribute is defined.

@marcmo
Copy link
Contributor Author

marcmo commented May 7, 2021

Hi @nicholastmosher, thanks for laying out your thoughts on this.
After reading about the ctor library and how it works, it seems dangerous to expose a hook so that your users can add any function.
OTOH if the initialization code fails, it will fail at startup and thus should be easy to debug.
As for how to implement a static that holds a reference to a function...I tried but failed... don't know how to implement that!
would be a nice way to enable replacing the default-logger initialization that way

@nicholastmosher
Copy link
Contributor

I think when I suggested that solution, I was looking at some code from tracing as inspiration: https://github.com/tokio-rs/tracing/blob/master/tracing-core/src/dispatch.rs#L197-L202

They have a static GLOBAL_DISPATCH, where "dispatch" seems to refer to a reference to a particular "Collector", which is their interface for receiving log events. I will need to come back and read that more thoroughly, but I wanted to post it in case it can help you think through what an implementation here might look like

@Badel2
Copy link

Badel2 commented Aug 13, 2021

Hi, I also would like to setup a custom logger. The problem is that the log library has no way to "unset" a logger, so we are stuck with the logger from here. I like the solution to add a #[node_bindgen(init)] attribute, but in my case I need to pass some extra arguments to that function so I would use it like this:

#[node_bindgen(init)]
pub fn no_init() {
    // Disable default logger
}

pub fn manual_init(args: Value) {
    // Setup logger manually, using options from args
}

And then just call manual_init manually from my code. So if this init attribute is hard to implement, some config option to disable the default logger would be enough for me.

@sehz
Copy link
Collaborator

sehz commented Aug 13, 2021

Thanks for suggestion. Will try to address this issue soon.

@sehz sehz added enhancement New feature or request nj-derive labels Aug 13, 2021
@sehz
Copy link
Collaborator

sehz commented Aug 13, 2021

@Badel2 What option do you need for init? It's hard to pass argument. It would be simpler if pick up form env variable.

If we go with init attribute. then it just matter of copy/paste function in the init_module

@Badel2
Copy link

Badel2 commented Aug 14, 2021

Actually I need to pass a function, like this:

let rust_addon = require("../../rust-dist");
rust_addon.manual_init(function(msg) {
    console.log(msg);
});

I tried to get a reference to console.log using JsEnv, but that didn't work.

Another possibility is to allow using RUST_LOG=off to disable the default logger, you would need to modify this function:

pub fn init_logger() {
fluvio_future::subscriber::init_logger();
}

Into something like

if std::env("RUST_LOG") == "off" {
    // Do not initialize logger here, to allow user to set a custom logger
} else {
    fluvio_future::subscriber::init_logger(); 
}

marcmo added a commit to marcmo/node-bindgen that referenced this issue Aug 19, 2021
- fixes infinyon#134
- Provide macro to run code on initialization without introducing
additional direct dependencies to client projects.
- without this global initialization applications need to setup their
own logging infrastructure at startup
- Adapted function example with the use of a custom logger
marcmo added a commit to marcmo/node-bindgen that referenced this issue Sep 28, 2021
- fixes infinyon#134
- Provide macro to run code on initialization without introducing
additional direct dependencies to client projects.
- without this global initialization applications need to setup their
own logging infrastructure at startup
- Adapted logger example with the use of a custom logger
@bors bors bot closed this as completed in 3e7a2c9 Sep 30, 2021
@sehz
Copy link
Collaborator

sehz commented Sep 30, 2021

Thanks to @marcmo, this issue is resolved. Should do release? I think only minor bump version is necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request nj-derive
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants