Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

📢 Thoughts on this report? Let us know!

@rina23q It seems that the execution order of the plugins is not controlled, as the order keeps on changing, below shows the output from two different runs.

Run 1

$ sudo tedge diag collect --plugin-dir ./tedge-diag-plugins/plugins
Executing ./tedge-diag-plugins/plugins/02_os.sh... ✓
Executing ./tedge-diag-plugins/plugins/04_workflow.sh... ✓
Executing ./tedge-diag-plugins/plugins/07_mosquitto.sh... ✓
Executing ./tedge-diag-plugins/plugins/05_entities.sh... ✓
Executing ./tedge-diag-plugins/plugins/03_mqtt.sh... ✓
Executing ./tedge-diag-plugins/plugins/00_template.sh... ✓
Executing ./tedge-diag-plugins/plugins/01_tedge.sh... ✓
Executing ./tedge-diag-plugins/plugins/06_internal.sh... ✓
Total 8 executed: 8 completed, 0 failed, 0 skipped
Diagnostic information saved to /tmp/tedge-diag-2025-05-13T15:49:03.tar.gz

Run 2

$ sudo tedge diag collect --plugin-dir ./tedge-diag-plugins/plugins
Executing ./tedge-diag-plugins/plugins/00_template.sh... ✓
Executing ./tedge-diag-plugins/plugins/04_workflow.sh... ✓
Executing ./tedge-diag-plugins/plugins/03_mqtt.sh... ✓
Executing ./tedge-diag-plugins/plugins/07_mosquitto.sh... ✓
Executing ./tedge-diag-plugins/plugins/06_internal.sh... ✓
Executing ./tedge-diag-plugins/plugins/02_os.sh... ✓
Executing ./tedge-diag-plugins/plugins/01_tedge.sh... ✓
Executing ./tedge-diag-plugins/plugins/05_entities.sh... ✓
Total 8 executed: 8 completed, 0 failed, 0 skipped
Diagnostic information saved to /tmp/tedge-diag-2025-05-13T15:49:56.tar.gz

Expected

• The plugins should be sorted alphabetically and then executed in order (serially)

@rina23q Overall the feature works nicely. It was able to guess how to use it by simply using the tab completion for the new command, and looking at the existing plugins (a really good sign that it has a solid existing UX).

Below are just some minor improvements/suggestions to improve the overall UX (mostly polish).

1. Do we need to split the plugin's standard output and standard error into two different files? (e.g. out.log and err.log). Since the plugin can write the output to file, then the plugins might already write to an additional file. Having a combined out.log and err.log will just make it easier for the person reading these files to know where to look

2. delete empty out.log and err.log files. Removing them helps the user to see if there is any useful information in them or not (if they can't see the file size)

3. If plugin (which matches the given pattern) is not executable, then it would be useful to print a warning as this is most likely a mistake.

   WARN: Skipping plugin as it is not executable. file: ./tedge-diag-plugins/plugins/04_example.sh

4. The temp directory should be cleaned up after the tarball is created. It might be useful to add a flag to control whether or not to keep the directory (helpful for debugging), or just to create the directory and not the tarball?

5. folder and tarball are named differently, e.g. the folder has a timestamp with milliseconds, and the tarball only has second resolution. It is expected that both the folder and the tarball use the same naming convention (whole second resolution is enough in this case)

   $ ls -l /tmp/                        
   total 256
   drwxr-xr-x 10 root root   200 May 13 15:48 tedge-diag-2025-05-13T15:48:02.914068322Z
   -rw-r--r--  1 root root 24910 May 13 15:48 tedge-diag-2025-05-13T15:48:02.tar.gz

6. Writing a list of which plugins that was executed and storing it in the output folder would be useful for someone analyzing the diagnostic logs on what was executed and what was skipped (as there might also be a bug in the plugin itself)

   For example something like below:

   file: /tmp/tedge-diag-2025-05-13T16:06:27/collection.log

   Executing ./tedge-diag-plugins/plugins/03_mqtt.sh... ✓
   Executing ./tedge-diag-plugins/plugins/02_os.sh... ✓
   Executing ./tedge-diag-plugins/plugins/02_reuben.sh... ✓
   ERROR: ./tedge-diag-plugins/plugins/02_reuben.sh failed with exit status: exit status: 1
   Executing ./tedge-diag-plugins/plugins/01_tedge.sh... ✓
   Executing ./tedge-diag-plugins/plugins/05_entities.sh... ✓
   Executing ./tedge-diag-plugins/plugins/06_internal.sh... ✓
   Executing ./tedge-diag-plugins/plugins/07_mosquitto.sh... ✓
   Executing ./tedge-diag-plugins/plugins/04_workflow.sh... ✓
   Executing ./tedge-diag-plugins/plugins/00_template.sh... ✓
   Total 9 executed: 8 completed, 1 failed, 0 skipped

7. What is the default colour configured for a log message at the INFO level. At the moment it is looks like yellow which would indicate a warning rather than an informational message. But this could be my terminal colour settings and I don't have a warning message to compare.

   INFO: ./tedge-diag-plugins/plugins/02_reuben.sh is marked skipped
   

@reubenmiller
Thanks for a lot of feedback. Let me reply to your suggestions :)

The plugins should be sorted alphabetically and then executed in order (serially)

Done! Switched from HashSet to BTreeSet.

1. Do we need to split the plugin's standard output and standard error into two different files?

No, and since now the code uses the same plugin APIs as software management and others, the output log file keeps both stdout and stderr together. I updated the specification markdown file accordingly.

2. delete empty out.log and err.log files. Removing them helps the user to see if there is any useful information in them or not (if they can't see the file size)

Related to the point 1, since the plugin API outputs some more information other than stdout and stderr, empty file should not be created now.

3. If plugin (which matches the given pattern) is not executable, then it would be useful to print a warning as this is most likely a mistake.

Changed to output a warning message as below. I'm still now searching for a better crate to address error/warn/info messages in a nice way, the output color and format may be changed in a later point.

WARN: Skipping plugin as it is not executable. file: "/home/rina/Work/thin-edge/tedge-diag-plugins/plugins/100_dummy.sh"

4. The temp directory should be cleaned up after the tarball is created. It might be useful to add a flag to control whether or not to keep the directory (helpful for debugging), or just to create the directory and not the tarball?

I added a new option --clean-tmp-output. By default true. If false, the temporary directory won't be removed.

5. folder and tarball are named differently, e.g. the folder has a timestamp with milliseconds, and the tarball only has second resolution. It is expected that both the folder and the tarball use the same naming convention (whole second resolution is enough in this case)

Previously, I used RFC3339 format (e.g. 1985-04-12T23:20:50.52Z), but this caused several problems.

• . confuses the program which indicates an extension
• : confuses tar command to decompress, as : is considered hostname
  So, I decided to use this custom format. [year]-[month]-[day]_[hour]-[minute]-[second]

drwxrwxr-x 11 rina rina     4096 May 20 17:08 tedge-diag-2025-05-20_15-08-42
-rw-rw-r--  1 rina rina   218946 May 20 17:08 tedge-diag-2025-05-20_15-08-42.tar.gz

6. Writing a list of which plugins that was executed and storing it in the output folder would be useful for someone analyzing the diagnostic logs on what was executed and what was skipped (as there might also be a bug in the plugin itself)

I haven't done it yet. I'll put it in todo.

7. What is the default colour configured for a log message at the INFO level. At the moment it is looks like yellow which would indicate a warning rather than an informational message. But this could be my terminal colour settings and I don't have a warning message to compare.

I think green or blue. Yellow was definitely wrong. Related to the point 3, it's still in todo to improve output messages.

Robot Results

To track the progress of #3608 (review)

• 1. Won't do here since it's the bug in the main branch
• 2. 6df225c
• 3. f8fdd36
• 4. da6ecfa
• 5. ae6ffb8
• 6. The default value for both timeouts are already set to 60s and these values are displayed by --help.

      --timeout <TIMEOUT>
          Timeout for a graceful plugin shutdown
          
          [default: 60s]

      --forceful-timeout <FORCEFUL_TIMEOUT>
          Timeout for forced termination, starting after a graceful timeout expires
          
          [default: 60s]

• 7. As well as 6., I think this one was fixed already. Just in case, I added a check no stdout was recorded in the Robot test. 4c466b2