Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grafana Integration #425

Open
wants to merge 27 commits into
base: ros2
Choose a base branch
from
Open

Grafana Integration #425

wants to merge 27 commits into from

Conversation

dwffls
Copy link

@dwffls dwffls commented Jan 8, 2025

Draft PR based on #401
Creating a new node that can export all diagnostics to telegraf or influxdb to be used with tools like grafana.

@dwffls dwffls changed the title Influx db Grafana Integration Jan 8, 2025
@dwffls
Copy link
Author

dwffls commented Jan 8, 2025

Build doesn't complete as it misses the dependency to cURL, @ct2034 could you help me with this?

diagnostic_remote_logging/package.xml Outdated Show resolved Hide resolved
diagnostic_remote_logging/package.xml Outdated Show resolved Hide resolved
@dwffls
Copy link
Author

dwffls commented Jan 8, 2025

Yup that fixes it

@ct2034
Copy link
Collaborator

ct2034 commented Jan 8, 2025

Thanks for your work. This looks very good.
I think a big part that is currently missing is tests. What is your plan for them? I am not sure if spinning up an actual grafana instance may be overkill, but maybe you can mock the receiving interface.

@dwffls
Copy link
Author

dwffls commented Jan 9, 2025

I've added unit tests for the conversion from diagnostic_msgs to the Influx-compatible line protocol. This is the primary focus of the package and, in my opinion, the area most prone to potential errors.

The remaining code consists of a lightweight node that connects to an InfluxDB or Telegraf instance using basic curl functions. I'm uncertain how to effectively test this part, as errors are more likely to originate from changes on the other side of the connection. As such, I'm questioning whether adding tests here would provide significant value.

@ct2034 ct2034 added enhancement This tackles a new feature of the code (and not a bug) ros2 PR tackling a ROS2 branch labels Jan 13, 2025
diagnostic_remote_logging/README.md Outdated Show resolved Hide resolved
diagnostic_remote_logging/package.xml Outdated Show resolved Hide resolved
diagnostic_remote_logging/package.xml Outdated Show resolved Hide resolved
diagnostic_remote_logging/CMakeLists.txt Outdated Show resolved Hide resolved
diagnostic_remote_logging/CMakeLists.txt Outdated Show resolved Hide resolved
/*********************************************************************
* Software License Agreement (BSD License)
*
* Copyright (c) 2009, Willow Garage, Inc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change

Copy link
Author

@dwffls dwffls Jan 15, 2025

Choose a reason for hiding this comment

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

You mean the year right? Or also the copyright holder? If so what should that be?
This is the first time i've done anything like this

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be the copyright holder. And that is you or your company, depending on how you handle these things.

diagnostic_remote_logging/src/influxdb.cpp Show resolved Hide resolved
diagnostic_remote_logging/test/influx_line_protocol.cpp Outdated Show resolved Hide resolved
@ct2034
Copy link
Collaborator

ct2034 commented Jan 15, 2025

And please add your package to [.github/workflows/lint.yaml] and [.github/workflows/test.yaml]

Copy link
Collaborator

@ct2034 ct2034 left a comment

Choose a reason for hiding this comment

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

Please also look at the linter feedback


This package provides the `influx` node, which listens to diagnostic messages and integrates with InfluxDB v2 for monitoring and visualization. Specifically, it subscribes to the [`diagnostic_msgs/DiagnosticArray`](https://index.ros.org/p/diagnostic_msgs) messages on the `/diagnostics_agg` topic and the [`diagnostic_msgs/DiagnosticStatus`](https://index.ros.org/p/diagnostic_msgs) messages on the `/diagnostics_toplevel_state` topic. The node processes these messages, sending their statistics and levels to an [`InfluxDB`](http://influxdb.com) database, enabling use with tools like [`Grafana`](https://grafana.com).

As of now we only support InfluxDB v2, for support with older versions please use a proxy like [`Telegraf`](https://www.influxdata.com/time-series-platform/telegraf/). See section [Telegraf](## Using a Telegraf Proxy) for an example on how to setup.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry. There is still something broken with the internal link. https://stackoverflow.com/questions/27981247/github-markdown-same-page-link

Comment on lines +40 to +41
#### InfluxDB Configuration

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this section now empty? Please think about the structure of the readme again

/*********************************************************************
* Software License Agreement (BSD License)
*
* Copyright (c) 2025, Willow Garage, Inc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have to add yourself and / or your company here instead of Willow Garage.
(Applies to all headers files with headers)

<version>4.3.1</version>
<description>diagnostic_remote_logging</description>
<maintainer email="[email protected]">Daan Wijffels</maintainer>
<license>>BSD-3-Clause</license>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<license>>BSD-3-Clause</license>
<license>BSD-3-Clause</license>

/*********************************************************************
* Software License Agreement (BSD License)
*
* Copyright (c) 2009, Willow Garage, Inc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be the copyright holder. And that is you or your company, depending on how you handle these things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This tackles a new feature of the code (and not a bug) ros2 PR tackling a ROS2 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants