Log memory usage of the agent and the mapper #2693

didier-wenzek · 2024-02-08T16:18:52Z

Proposed changes

Log memory usage of the agent and the mapper.
The default interval is of 60 seconds.

This can be changed using the --log-memory-interval flag which sets the logging interval in seconds.

$ target/debug/tedge-mapper --log-memory-interval 5 c8y
$ sudo target/debug/tedge-agent --log-memory-interval 5

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

#2692 (comment)

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

codecov · 2024-02-08T16:30:11Z

Codecov Report

Attention: 55 lines in your changes are missing coverage. Please review.

Comparison is base (449f42b) 76.9% compared to head (e783cfb) 76.9%.
Report is 1 commits behind head on main.

Additional details and impacted files

Files	Coverage Δ
.../tedge_config/src/tedge_config_cli/tedge_config.rs	`81.1% <75.0%> (-0.1%)`	⬇️
crates/core/tedge_mapper/src/lib.rs	`0.0% <0.0%> (ø)`
crates/common/tedge_config/src/lib.rs	`0.0% <0.0%> (ø)`
crates/core/tedge_mapper/src/main.rs	`6.2% <8.3%> (+6.2%)`	⬆️
crates/core/tedge_agent/src/main.rs	`6.2% <7.6%> (+6.2%)`	⬆️
crates/core/tedge/src/main.rs	`45.5% <4.0%> (-15.9%)`	⬇️

... and 3 files with indirect coverage changes

github-actions · 2024-02-08T16:43:13Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
391	0	3	391	100	55m28.415999999s

crates/core/tedge_agent/src/lib.rs

crates/core/tedge_agent/src/main.rs

albinsuresh · 2024-02-09T05:57:36Z

Cargo.toml

@@ -56,6 +56,7 @@ c8y_http_proxy = { path = "crates/extensions/c8y_http_proxy" }
 c8y_log_manager = { path = "crates/extensions/c8y_log_manager" }
 c8y_mapper_ext = { path = "crates/extensions/c8y_mapper_ext" }
 camino = "1.1"
+cap = "0.1"


Side commentary: Came across this sysinfo crate that covers a lot more parameters like CPU usage. Something we can consider in the future, for advanced monitoring.

The purpose is different. Sysinfo observes the operating system, while here the point is to monitor the amount of memory actually allocated by tedge on the heap.

albinsuresh · 2024-02-09T05:59:35Z

crates/core/tedge_agent/src/lib.rs

+
+    /// Interval at which the memory usage is logged (in seconds)
+    #[clap(long = "log-memory-interval", default_value = "60")]
+    pub log_memory_interval: u64,


Is this a short term addition or something permanent? If permanent, any specific reason why this is a command-line argument and not tedge config setting (other than convenience and simplicity)? No objections here, but just trying to understand the rationale behind that choice.

The idea is to set this right when the process starts - ideally before any thin-edge processing.

I see this as a new feature. This can possibly be extended with:

the number of threads (currently we use the default i.e one thread per core),

a cap on allocated memory (crashing if the process starts to go wild).

Supporting a config would make it easier to enable on devices under test...as adding a flag is more difficult as we would need to use systemctl edit ... to enable this feature.

I will give a try to a config setting. This is definitely more convenient. However, not so straightforward to implement.

Supporting a config would make it easier to enable on devices under test...as adding a flag is more difficult as we would need to use systemctl edit ... to enable this feature.

Done with 4d19e41

The new setting is run.log_memory_interval with a default of 5 minutes.

didier-wenzek · 2024-02-09T09:30:28Z

crates/core/tedge_agent/src/main.rs


 #[tokio::main]
-async fn main() -> Result<(), anyhow::Error> {
+async fn main() -> anyhow::Result<()> {


We have to consider deprecating this main and enforcing the use of a tedge -agent as a symlink to tedge.

Here is a ticket for that: #2696

didier-wenzek · 2024-02-09T16:57:33Z

crates/core/tedge/src/main.rs

+            let tedge_config = tedge_config::load_tedge_config(&opt.config_dir)?;
+            block_on_with(
+                tedge_config.run.log_memory_interval.duration(),
+                tedge_mapper::run(opt),
+            )


I'm not very happy with that, i.e. loading the config without passing it to tedge_mapper::run().

Doing so is not straightforward as each of the actors used by the mapper and the agent have specific way to load the config, with or without a reference to the config-dir and other weirdness including duplicated paths to the same setting. It would be good to simplify all that in a follow up PR.

The default interval is of 60 seconds. This can be changed using the --log-memory-interval flag which sets the logging interval in seconds. ``` $ target/debug/tedge-mapper --log-memory-interval 5 c8y $ sudo target/debug/tedge-agent --log-memory-interval 5 ``` Signed-off-by: Didier Wenzek <[email protected]>

reubenmiller

Looks good.

didier-wenzek requested review from albinsuresh, jarhodes314, rina23q and reubenmiller as code owners February 8, 2024 16:18

didier-wenzek requested a review from Bravo555 February 8, 2024 16:19

didier-wenzek temporarily deployed to Test Pull Request February 8, 2024 16:26 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto February 8, 2024 16:32 — with GitHub Actions Inactive

Bravo555 approved these changes Feb 8, 2024

View reviewed changes

crates/core/tedge_agent/src/lib.rs Outdated Show resolved Hide resolved

didier-wenzek force-pushed the feat/trace-memory-usage branch from 4b0a41d to 549d6ec Compare February 8, 2024 19:28

didier-wenzek temporarily deployed to Test Pull Request February 8, 2024 19:35 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto February 8, 2024 19:39 — with GitHub Actions Inactive

didier-wenzek commented Feb 8, 2024

View reviewed changes

crates/core/tedge_agent/src/main.rs Outdated Show resolved Hide resolved

albinsuresh reviewed Feb 9, 2024

View reviewed changes

didier-wenzek temporarily deployed to Test Pull Request February 9, 2024 09:22 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto February 9, 2024 09:28 — with GitHub Actions Inactive

didier-wenzek commented Feb 9, 2024

View reviewed changes

didier-wenzek temporarily deployed to Test Pull Request February 9, 2024 16:47 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto February 9, 2024 16:52 — with GitHub Actions Inactive

didier-wenzek commented Feb 9, 2024

View reviewed changes

didier-wenzek force-pushed the feat/trace-memory-usage branch from 4d19e41 to e783cfb Compare February 9, 2024 17:27

reubenmiller approved these changes Feb 9, 2024

View reviewed changes

didier-wenzek temporarily deployed to Test Pull Request February 9, 2024 17:35 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto February 9, 2024 17:40 — with GitHub Actions Inactive

didier-wenzek added this pull request to the merge queue Feb 9, 2024

Merged via the queue into thin-edge:main with commit 5ce44d6 Feb 9, 2024
20 checks passed

reubenmiller mentioned this pull request Feb 14, 2024

fix: disable memory logging by default to avoid log spamming #2712

Merged

11 tasks

didier-wenzek deleted the feat/trace-memory-usage branch February 15, 2024 09:33

reubenmiller added this to the 1.0.1 milestone Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log memory usage of the agent and the mapper #2693

Log memory usage of the agent and the mapper #2693

didier-wenzek commented Feb 8, 2024 •

edited

Loading

codecov bot commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

albinsuresh Feb 9, 2024

didier-wenzek Feb 9, 2024

albinsuresh Feb 9, 2024

didier-wenzek Feb 9, 2024

reubenmiller Feb 9, 2024

didier-wenzek Feb 9, 2024

didier-wenzek Feb 9, 2024 •

edited

Loading

didier-wenzek Feb 9, 2024

didier-wenzek Feb 9, 2024

didier-wenzek Feb 9, 2024

reubenmiller left a comment

Log memory usage of the agent and the mapper #2693

Log memory usage of the agent and the mapper #2693

Conversation

didier-wenzek commented Feb 8, 2024 • edited Loading

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Feb 8, 2024 • edited Loading

Codecov Report

github-actions bot commented Feb 8, 2024 • edited Loading

Robot Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didier-wenzek Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reubenmiller left a comment

Choose a reason for hiding this comment

didier-wenzek commented Feb 8, 2024 •

edited

Loading

codecov bot commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

didier-wenzek Feb 9, 2024 •

edited

Loading