To be honest, initially I discarded the package upon reading the README.md.
But re-reading it again I also do recognize the potential. So I think it’s only fair to mention what put me off initially since you are asking for feedback ![]()
Custom installation instructions
# Add InfluxDB repository
curl -s https://repos.influxdata.com/influxdata-archive_compat.key | sudo apt-key add -
echo "deb https://repos.influxdata.com/ubuntu $(lsb_release -cs) stable" | sudo tee /etc/apt/sources.list.d/influxdata.list
# Install Telegraf
sudo apt update
sudo apt install telegraf
So that is not going to happen for us. This is something every developer in our company needs to do. Or we need to script around it.
Also our deployment pipelines do not have these instructions. Again implementing or scripting.
Might not seem like a big deal, but if every package does it like this, it becomes a mess. Also when we remove a dependency we need to crawl our scripting to see if anything custom was made. You get the point.
Standard ROS way of working is:
rosdep install --ignore-src --from-paths src/telegraf_resource_monitor
This means you should probably create a vendor package telegraf_vendor and have an exec_depend on it.
And for convienence a launchfile would be nice which I could just use as an example which launches everything I need to get /diagnostics and for example the CPU. This includes launching whatever process telegraf needs.
Spawning lots of topics
From the README:
/cpu/cpu0
/cpu/cpu1
/cpu/cpu2
/cpu/cpu3
/cpu/cpu_total
/disk/root
/mem
/procstat/telegraf_resource_monitor
/sensors/acpitz_acpi_0/temp1
/sensors/amdgpu_pci_0400/edge
/sensors/amdgpu_pci_0400/slowppt
/sensors/amdgpu_pci_0400/vddgfx
/sensors/amdgpu_pci_0400/vddnb
/sensors/bat1_acpi_0/in0
/sensors/iwlwifi_1_virtual_0/temp1
/sensors/k10temp_pci_00c3/tctl
/sensors/nvme_pci_0100/composite
/sensors/nvme_pci_0100/sensor_1
But like @peci1 we’re only interested in /diagnostics for now. So this is clutting the topic list a lot.
I think the creation of N topics should be made optional. Or ideally we can pick some. For example we might have a special interest in the battery one, not the rest.
This probably does mean you need to find another way to interface between telegraf_resource_monitor and resource_diagnostics_updater, but not sure.