Skip to content

Add --log-level flag#3205

Merged
Bravo555 merged 3 commits intothin-edge:mainfrom
Bravo555:fix/2089/log-level-flag
Oct 28, 2024
Merged

Add --log-level flag#3205
Bravo555 merged 3 commits intothin-edge:mainfrom
Bravo555:fix/2089/log-level-flag

Conversation

@Bravo555
Copy link
Copy Markdown
Member

@Bravo555 Bravo555 commented Oct 24, 2024 •
edited
Loading

Proposed changes

Add a --log-level flag that sets the verbosity of reported logs.

In contrast to --debug, which enables debug logs in addition to the default error, warn, and info logs, --log-level sets the maximum verbosity, i.e. when set to error print only errors, if set to warn print warnings and errors, etc. and when set to trace, print all the logs.

If --log-level is selected, this value is used and takes precedence over --debug. RUST_LOG env variable still overrides the flag.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

There were 4 places where an identical change was necessary, perhaps we should consider how to extract all the common CLI flags into a shared component.

Contains a small refactor to logging initialization that eliminates the duplication. Dedicated PR #3208

@Bravo555 Bravo555 marked this pull request as ready for review October 24, 2024 14:53
@Bravo555 Bravo555 changed the title Add --level flag Add --level flag Oct 24, 2024
@Bravo555 Bravo555 force-pushed the fix/2089/log-level-flag branch from 8f6d522 to 44c499c Compare October 24, 2024 14:58
@Bravo555 Bravo555 changed the title Add --level flag Add --log-level flag Oct 24, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 24, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 50 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...mon/tedge_config/src/system_services/log_config.rs 0.00% 30 Missing ⚠️
crates/core/tedge_agent/src/lib.rs 0.00% 5 Missing ⚠️
crates/core/tedge_mapper/src/lib.rs 0.00% 5 Missing ⚠️
crates/core/tedge_watchdog/src/lib.rs 0.00% 5 Missing ⚠️
plugins/c8y_firmware_plugin/src/lib.rs 0.00% 5 Missing ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

albinsuresh
albinsuresh reviewed Oct 25, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

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

A few copy paste issues.

crates/core/tedge_mapper/src/lib.rs Outdated

let log_level = match log_level {
Some(log_level) => log_level,
None => get_log_level("tedge-agent", &tedge_config_location.tedge_config_root_path)?,
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh Oct 25, 2024

Choose a reason for hiding this comment

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

Suggested change
None => get_log_level("tedge-agent", &tedge_config_location.tedge_config_root_path)?,
None => get_log_level("tedge-mapper", &tedge_config_location.tedge_config_root_path)?,

crates/core/tedge_watchdog/src/lib.rs Outdated

let log_level = match log_level {
Some(log_level) => log_level,
None => get_log_level("tedge-agent", &tedge_config_location.tedge_config_root_path)?,
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh Oct 25, 2024

Choose a reason for hiding this comment

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

Suggested change
None => get_log_level("tedge-agent", &tedge_config_location.tedge_config_root_path)?,
None => get_log_level("tedge-watchdog", &tedge_config_location.tedge_config_root_path)?,

plugins/c8y_firmware_plugin/src/lib.rs Outdated

let log_level = match log_level {
Some(log_level) => log_level,
None => get_log_level("tedge-agent", &tedge_config_location.tedge_config_root_path)?,
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh Oct 25, 2024

Choose a reason for hiding this comment

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

Suggested change
None => get_log_level("tedge-agent", &tedge_config_location.tedge_config_root_path)?,
None => get_log_level("c8y-firmware-plugin", &tedge_config_location.tedge_config_root_path)?,

didier-wenzek
didier-wenzek reviewed Oct 25, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

There were 4 places where an identical change was necessary, perhaps we should consider how to extract all the common CLI flags into a shared component.

Could this be implemented in tedge::main.rs, the multi-call entry for the agent and the mapper?

@reubenmiller
Copy link
Copy Markdown
Contributor

reubenmiller commented Oct 25, 2024 •
edited
Loading

If --log-level is selected, this value is used and takes precedence over --debug. RUST_LOG env variable still overrides the flag.

Flags should always take precedence over environment variable values...generally flags are often used by users to override the environment which they usually have little control over.

The first non-empty value should be taken (in order):

  1. Flag
  2. Environment variable
  3. Configuration file

@reubenmiller reubenmiller added theme:cli Theme: cli related topics theme:troubleshooting Theme: Troubleshooting and remote control labels Oct 25, 2024
@Bravo555 Bravo555 mentioned this pull request Oct 28, 2024
Merged
11 tasks
@Bravo555 Bravo555 force-pushed the fix/2089/log-level-flag branch from 44c499c to 62aa224 Compare October 28, 2024 11:37
@Bravo555 Bravo555 temporarily deployed to Test Pull Request October 28, 2024 11:37 — with GitHub Actions Inactive
@Bravo555
Copy link
Copy Markdown
Member Author

Bravo555 commented Oct 28, 2024 •
edited
Loading

Flags should always take precedence over environment variable values...generally flags are often used by users to override the environment which they usually have little control over.

The first non-empty value should be taken (in order):

  1. Flag
  2. Environment variable
  3. Configuration file

Made a dedicated PR that fixes that mistake: #3208

Could this be implemented in tedge::main.rs, the multi-call entry for the agent and the mapper?

That would probably require some more changes because for every component we use a separate CLI parser, but for now #3208 eliminates some duplication.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 28, 2024 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
523 0 2 523 100 1h32m50.914102999s

@Bravo555
Make --debug flag override RUST_LOG
5657a13 
Flags should always take precedence over environment variables because
are often used by users to override the environment which they usually
have little control over.

To implement this change it was necessary to refactor the code a little
bit as previously set_log_level function was not aware if desired log
level was specifically requested by the user (--debug flag) or just the
default (INFO level). As such, the new log_init function takes into
account all sources that decide what log level we should select: CLI
options, RUST_LOG env variable, and tedge configuration file.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@Bravo555
Remove all calls to set_log_level in tests
537e02d 
Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@Bravo555
Add --log-level flag
cc41097 
Add a --log-level flag that sets the verbosity of reported logs.

In contrast to --debug, which enables debug logs in addition to the
default error, warn, and info logs, --log-level sets the maximum
verbosity, i.e. when set to error print only errors, if set to warn
print warnings and errors, etc. and when set to trace, print all the
logs.

If --log-level is selected, this value is used and takes precedence over
--debug. RUST_LOG env variable still overrides the flag.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@Bravo555 Bravo555 force-pushed the fix/2089/log-level-flag branch from 62aa224 to cc41097 Compare October 28, 2024 14:59
@Bravo555 Bravo555 temporarily deployed to Test Pull Request October 28, 2024 14:59 — with GitHub Actions Inactive
@Bravo555
Copy link
Copy Markdown
Member Author

Bravo555 commented Oct 28, 2024

Although, would it be a better user experience to only allow using either --debug OR --log-level, but not both? It might eliminate some confusion of which overrides the other.

didier-wenzek
didier-wenzek approved these changes Oct 28, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved

@didier-wenzek
Copy link
Copy Markdown
Contributor

didier-wenzek commented Oct 28, 2024

Although, would it be a better user experience to only allow using either --debug OR --log-level, but not both? It might eliminate some confusion of which overrides the other.

Indeed these two flags overlap. But, I don't see that as a big deal.

@Bravo555 Bravo555 added this pull request to the merge queue Oct 28, 2024
Merged via the queue into thin-edge:main with commit ad03ca3 Oct 28, 2024
@Bravo555 Bravo555 mentioned this pull request Nov 4, 2024
Closed
@Bravo555 Bravo555 mentioned this pull request Nov 18, 2024
Merged
11 tasks
@reubenmiller reubenmiller added this to the 1.4.0 milestone Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albinsuresh albinsuresh albinsuresh left review comments

@didier-wenzek didier-wenzek didier-wenzek approved these changes

@jarhodes314 jarhodes314 Awaiting requested review from jarhodes314 jarhodes314 is a code owner

@rina23q rina23q Awaiting requested review from rina23q rina23q is a code owner

@reubenmiller reubenmiller Awaiting requested review from reubenmiller reubenmiller is a code owner

Assignees

No one assigned

Labels

theme:cli Theme: cli related topics theme:troubleshooting Theme: Troubleshooting and remote control

Projects

None yet

Milestone

1.4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@Bravo555 @reubenmiller @didier-wenzek @albinsuresh