Add `--log-level` flag #3205

Bravo555 · 2024-10-24T14:51:35Z

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

Control command's log level via a flag #2089

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

codecov · 2024-10-24T15:09:22Z

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

A few copy paste issues.

albinsuresh · 2024-10-25T06:43:08Z

crates/core/tedge_mapper/src/lib.rs

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


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)?,

albinsuresh · 2024-10-25T06:43:50Z

crates/core/tedge_watchdog/src/lib.rs

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


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)?,

albinsuresh · 2024-10-25T06:44:08Z

plugins/c8y_firmware_plugin/src/lib.rs

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


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

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 · 2024-10-25T12:55:37Z

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):

Flag
Environment variable
Configuration file

Bravo555 · 2024-10-28T11:53:15Z

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):

Flag

Environment variable

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 · 2024-10-28T13:13:34Z

Robot Results

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

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 <[email protected]>

Signed-off-by: Marcel Guzik <[email protected]>

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 <[email protected]>

Bravo555 · 2024-10-28T15:12:10Z

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

Approved

didier-wenzek · 2024-10-28T17:08:00Z

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 requested a deployment to Test Pull Request October 24, 2024 14:51 — with GitHub Actions Waiting

Bravo555 marked this pull request as ready for review October 24, 2024 14:53

Bravo555 requested review from didier-wenzek, albinsuresh, jarhodes314 and rina23q as code owners October 24, 2024 14:53

Bravo555 changed the title ~~Add --level flag~~ Add --level flag Oct 24, 2024

Bravo555 force-pushed the fix/2089/log-level-flag branch from 8f6d522 to 44c499c Compare October 24, 2024 14:58

Bravo555 requested a deployment to Test Pull Request October 24, 2024 14:58 — with GitHub Actions Waiting

Bravo555 changed the title ~~Add --level flag~~ Add --log-level flag Oct 24, 2024

albinsuresh reviewed Oct 25, 2024

View reviewed changes

didier-wenzek reviewed Oct 25, 2024

View reviewed changes

reubenmiller added theme:cli Theme: cli related topics theme:troubleshooting Theme: Troubleshooting and remote control labels Oct 25, 2024

Bravo555 mentioned this pull request Oct 28, 2024

Make --debug flag override RUST_LOG #3208

Merged

11 tasks

Bravo555 force-pushed the fix/2089/log-level-flag branch from 44c499c to 62aa224 Compare October 28, 2024 11:37

Bravo555 requested a review from reubenmiller as a code owner October 28, 2024 11:37

Bravo555 temporarily deployed to Test Pull Request October 28, 2024 11:37 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto October 28, 2024 12:55 — with GitHub Actions Inactive

Bravo555 added 3 commits October 28, 2024 15:53

Remove all calls to set_log_level in tests

537e02d

Signed-off-by: Marcel Guzik <[email protected]>

Bravo555 force-pushed the fix/2089/log-level-flag branch from 62aa224 to cc41097 Compare October 28, 2024 14:59

Bravo555 temporarily deployed to Test Pull Request October 28, 2024 14:59 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto October 28, 2024 15:04 — with GitHub Actions Inactive

didier-wenzek approved these changes Oct 28, 2024

View reviewed changes

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
33 checks passed

Bravo555 mentioned this pull request Nov 4, 2024

Control command's log level via a flag #2089

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--log-level` flag #3205

Add `--log-level` flag #3205

Bravo555 commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

albinsuresh left a comment

albinsuresh Oct 25, 2024

albinsuresh Oct 25, 2024

albinsuresh Oct 25, 2024

didier-wenzek left a comment

reubenmiller commented Oct 25, 2024 •

edited

Loading

Bravo555 commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

Bravo555 commented Oct 28, 2024

didier-wenzek left a comment

didier-wenzek commented Oct 28, 2024

	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)?,

	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)?,

	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)?,

Add --log-level flag #3205

Add --log-level flag #3205

Conversation

Bravo555 commented Oct 24, 2024 • edited Loading

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Oct 24, 2024 • edited Loading

Codecov Report

albinsuresh left a comment

Choose a reason for hiding this comment

albinsuresh Oct 25, 2024

Choose a reason for hiding this comment

albinsuresh Oct 25, 2024

Choose a reason for hiding this comment

albinsuresh Oct 25, 2024

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

reubenmiller commented Oct 25, 2024 • edited Loading

Bravo555 commented Oct 28, 2024 • edited Loading

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

Robot Results

Bravo555 commented Oct 28, 2024

didier-wenzek left a comment

Choose a reason for hiding this comment

didier-wenzek commented Oct 28, 2024

Add `--log-level` flag #3205

Add `--log-level` flag #3205

Bravo555 commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

reubenmiller commented Oct 25, 2024 •

edited

Loading

Bravo555 commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading