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

fix: forward plugin logging output to Hipcheck core stderr/stdout #914

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KirilldogU
Copy link
Contributor

@KirilldogU KirilldogU commented Feb 12, 2025

Resolves #381.
This PR forwards the logging output of plugins to the stderr/stdout output of Hipcheck core.
It does this through:

  • Creating a feature in hipcheck_sdk to initialize an env_logger
  • Calling the init_logger() feature from each plugin
  • When plugins are launched as a child process, having them inherit the Stderr/Stdout of the parent Hipcheck core process.

It also adds docs in the sdk on how to use the init_logger() feature.

@KirilldogU
Copy link
Contributor Author

Would appreciate input on:

  • Is there a better place than main() for the init_logger() feature to be called in the plugins?
  • Is any of the added documentation too wordy or out of place? Specifically, the edit to the rust-sdk.md I am uncertain about as it is not essential to getting started with creating a plugin but it does relate to the features of the SDK.

@j-lanson
Copy link
Collaborator

My intuition would be to move the logger initialization into PluginServer::register() if the feature is enabled

@j-lanson
Copy link
Collaborator

Currently in the debug output the plugins appear as [2025-02-12T20:43:51Z INFO affiliation]. Would it be possible to have them appear as [2025-02-12T20:43:51Z INFO plugin::affiliation] so that we can control all the plugins' logging at once if we want? (e.g. HC_LOG=plugin=info)

@@ -32,6 +30,7 @@ pub struct PluginServer<P> {
impl<P: Plugin> PluginServer<P> {
/// Create a new plugin server for the provided plugin.
pub fn register(plugin: P) -> PluginServer<P> {
init_logger();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless init_logger already has idempotency, I might suggest a mechanism like a OnceLock to ensure we only initialize the logger once even if register() happens to be called multiple times (it shouldn't but we don't have a mechanism to prevent someone doing it)

@KirilldogU KirilldogU marked this pull request as draft February 13, 2025 15:08
@KirilldogU KirilldogU force-pushed the kusubyan/forward-plugin-logging branch from 5657c0a to 81ed06e Compare February 18, 2025 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Proper logging for plugins
2 participants