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

Feature: unify log level config #17286

Closed
youngsofun opened this issue Jan 14, 2025 · 6 comments · Fixed by #17444
Closed

Feature: unify log level config #17286

youngsofun opened this issue Jan 14, 2025 · 6 comments · Fixed by #17444
Assignees
Labels
C-feature Category: feature

Comments

@youngsofun
Copy link
Member

Current Behavior

Currently, the prefix_filter configuration is only supported for file logs. However, when a user enables this configuration, they likely expect it to apply to all log outputs, not just file logs.

Proposed Enhancement

Introduce a global prefix_filter configuration that applies to all logs. The existing prefix_filter for file logs can be retained for finer control, and it will override the global prefix_filter if specified. This approach ensures backward compatibility while making the configuration more intuitive.

Compatibility

  • The default value for the global prefix_filter will remain "databend_,openraft", maintaining existing behavior.
  • Setting the prefix_filter to an empty string will allow all log targets to be included.

Benefits

  • Simplifies log configuration for users by reducing the need to duplicate filters across different log types.
  • Retains fine-grained control for file-specific log filters when needed.
  • Provides a more consistent behavior for logging across the system.

Example

[log]
prefix_filter = "databend_"  # new config

[log.file]
level = "DEBUG"
format = "text"
dir = "./.databend/logs_1"
prefix_filter = "databend_,openraft" 
@youngsofun youngsofun added the C-feature Category: feature label Jan 14, 2025
@youngsofun
Copy link
Member Author

cc @wubx

@Xuanwo
Copy link
Member

Xuanwo commented Jan 16, 2025

Hi, this got me thinking about whether RUST_LOG itself supports filtering: https://docs.rs/env_logger/latest/env_logger/#enabling-logging

Perhaps we could simply extend the support for levels and pass it down to the underlying logging tools?

For example:

[log.file]
level = "DEBUG,databend_query=info"

@youngsofun
Copy link
Member Author

youngsofun commented Feb 12, 2025

After comprehensively reviewing the code related to logging, I've discovered that our level already effectively supports the env_logger format. For example, the current default setting for prefix_filter is "databend_,openraft". When prefix_filter is set to "" (which is the configuration in our CI) and level is set to "Warn,databend_=Info,openraft=Info", the results are identical.

Currently, we actually support two methods for setting filters. I'm not sure if this is a good approach. Fortunately, the prefix_filter option hasn't been documented yet, so we still have the opportunity to make adjustments.

If we uniformly adopt the env_filter fmt

  • Advantages: It is more standardized, clearer, and more flexible.
  • Disadvantages: Users need to be aware of prefixes such as databend_ and openraft when making configurations., most user only need log of databend it self, and only have need to change the level.

To - do items:

  1. Specify in the documentation that the env_logger format can be used.
  2. Modify the default value of all level from "INFO" to "Warn,databend_=Info,openraft=Info".

If we retaining both methods

To - do items:

  1. Enable support for prefix_filter in stderr log as well.
    • Note that we prefer to use independent configurations for each log, and the upper - level level is not documented any more. One reason is that since our default configuration values are not empty, it's difficult to distinguish between default values and user - intentionally set values. Additionally, the current practice where log.level overrides log.file.level is somewhat counter - intuitive.
  2. (Optional) Indicate in the documentation that the env_logger format can be used.
  3. (Optional) Document the prefix_filter, as an advanced option.

@Xuanwo @everpcpc @sundy-li @drmingdrmer @BohuTANG

@everpcpc
Copy link
Member

I prefer changing the default level to "warn,databend_=info,openraft=info"

@wubx
Copy link
Member

wubx commented Feb 12, 2025

It's best to keep it simple. If we already support env_logger, and this is a specification, we can just use this.

changing the default level to "warn,databend_=info,openraft=info,opendal=info"

@drmingdrmer
Copy link
Member

Unified setting would be better:)

@youngsofun youngsofun self-assigned this Feb 12, 2025
@youngsofun youngsofun changed the title Feature: support Global prefix_filter for All Logs Feature: unify log level config Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants