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

Hook up to swss-common logger #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yue-fred-gao
Copy link
Contributor

Why

SONiC uses config_db to keep logger configuration, which allows changing the configuration on the fly. On the back, swss-common logger subscribes to config_db logger table and notifies the corresponding component.

What this PR does

Hook up to swss-common logger to receive configuration update. In response to the change, dynamically changing log level in rust.

This PR can only be committed after sonic-net/sonic-swss-common#969

@mssonicbld
Copy link

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

.with_ansi(false)
.with_span_events(FmtSpan::NEW | FmtSpan::CLOSE);

tracing_subscriber::registry()

Choose a reason for hiding this comment

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

General question: Does this handle Rust's log macros? (info!, warn!, error!, etc.) Would log events go to the tracing subscriber, then into the swsscommon logger infra?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. swsscommon logger is only used to get log configuration from config_db and reconfigure logger when configuration changed. events do not go through swsscommon logger.

/// 2. If link_swsscommon_logger is set, use swsscommon logger to get log settings from config_db, which supports dynamic log level change
/// by modifying config_db. The settings include
/// - log_level: emerg, alert, crit, error, warn, notice, info, debug
/// - output: stdout, stderr, syslog. Rust doesn't support dynamically changing log output. We will only support default output (syslog in linux, file in windows)

Choose a reason for hiding this comment

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

Technically, even though Rust doesn't support having log::set_logger called multiple times, the implementation that's chosen can choose to redirect logs however it chooses to do so. For example, logforth can be used to have a chain of loggers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, we can reinitialize tracing system with a new subscriber, but this involves dropping the old subscriber and setting a new one. This will cause a short interruption in logging. Also, I see little use to change log destination. syslog should be sufficient so I didn't implement this.
I am not familiar with logforth. But looking it quickly, it seems not supporting reconfiguring log destination either. See https://docs.rs/logforth/latest/logforth/struct.Builder.html#method.apply

Choose a reason for hiding this comment

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

Ah, I hadn't seen that even logforth doesn't handle reconfiguration.

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