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

Support logging in geyser plugins #34101

Merged

Conversation

lijunwangs
Copy link
Contributor

Problem

The plugins need to be able to log as the rest of the validator in the validator logs. The plugin could use solana_logger::setup_with_defaul, unfortunately it is found doing that will cause the plugin not be unloaded from memory when the Library was dropped.

Summary of Changes

The change creates a new interface in the GeyserPlugin interface to allow the runtime to pass the logging configuration to the plugin.

fn setup_logger(&self, logger: &'static dyn log::Log, level: log::LevelFilter) -> Result<()>;
Which the plugin can choose to set it up:

    fn setup_logger(&self, logger: &'static dyn log::Log, level: log::LevelFilter) -> Result<()> {
        log::set_max_level(level);
        if let Err(err) = log::set_logger(logger) {
            return Err(GeyserPluginError::Custom(Box::new(err)));
        }
        Ok(())
    }

Fixes #

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Merging #34101 (6778005) into master (061883e) will increase coverage by 0.0%.
Report is 47 commits behind head on master.
The diff coverage is 66.6%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34101   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         812      812           
  Lines      219645   219660   +15     
=======================================
+ Hits       179922   179982   +60     
+ Misses      39723    39678   -45     

@t-nelson
Copy link
Contributor

does this really need to be so explicit? couldn't we dlopen the validator bin, dlsym the logger static, then dlclose the bin handle when unloading the plugin? i'm assuming that dangling implicit handle to the binary is what holds the plugin open on unload

@lijunwangs
Copy link
Contributor Author

does this really need to be so explicit? couldn't we dlopen the validator bin, dlsym the logger static, then dlclose the bin handle when unloading the plugin? i'm assuming that dangling implicit handle to the binary is what holds the plugin open on unload

We could, but I dislike that even more -- it forces the plugin to know the internal about the validator's implementations.

@lijunwangs
Copy link
Contributor Author

does this really need to be so explicit? couldn't we dlopen the validator bin, dlsym the logger static, then dlclose the bin handle when unloading the plugin? i'm assuming that dangling implicit handle to the binary is what holds the plugin open on unload

We could, but I dislike that even more -- it forces the plugin to know the internal about the validator's implementations.

@t-nelson any more concerns?

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

seems ok for a first pass.

i'm sure operators will expect the admin console's solana-validator set-log-level command to update the plugin(s) too. can be done in a followup

@lijunwangs
Copy link
Contributor Author

seems ok for a first pass.

i'm sure operators will expect the admin console's solana-validator set-log-level command to update the plugin(s) too. can be done in a followup

Good point. Will raise a new issue for it

@lijunwangs lijunwangs merged commit f211c86 into solana-labs:master Nov 29, 2023
32 checks passed
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