[feature] Allowed specifying additional options for InfluxDB backend (UDP) #458 #472

pandafy · 2022-12-23T12:04:52Z

Additional options can be specified to the TIMESERIES_DATABASE setting which are passed to the underlying backend library. This allows using UDP for writing to InfluxDB.
"openwisp_monitoring.db.backends.influxdb.client.DatabaseClient.write" will use "InfluxDBClient.write_points" instead of "InfluxDBClient.write" because the former allows writing data using the UDP protocol.

Closes #458

Checks:

I have manually tested the proposed changes
I have written new test cases to avoid regressions (if necessary)
I have updated the documentation (e.g. README.rst)

coveralls · 2022-12-23T12:19:52Z

Coverage: 98.908% (+0.005%) from 98.903% when pulling fa195eb on issues/458-influxdb-udp into 7f92b01 on master.

pandafy · 2022-12-29T11:31:52Z

I have found some caveats with using UDP for writing data to the InfluxDB:

UDP is used only for writing data to the InfluxDB. It requires adding additional configuration to the InfluxDB config.
It does not supports authentication.
It allows using only one "retention policy" that too has to be specified in the InfluxDB configuration.

nemesifier

I have mixed feelings about this, I do not feel comfortable in merging such a change while ignoring the test failures. See below.

I think the results which are coming out of the tests are encouraging on the perfomance side, but on the functionality side there's more work to do to merge this safely.

README.rst

tests/influxdb.conf

nemesifier · 2022-12-29T21:32:25Z

tests/openwisp2/settings.py

@@ -28,7 +28,15 @@
    'NAME': 'openwisp2',
    'HOST': os.getenv('INFLUXDB_HOST', 'localhost'),
    'PORT': '8086',
+    'OPTIONS': {'use_udp': True, 'udp_port': 8088},


I don't think we should switch to use UDP by default yet because this will add friction and impredictable consequences right now:

The fact that we cannot run tests against the UDP mode worries me, what if there's some critical part of the system which will not work as expected in this mode? How will we find out? It seems to me that this is the case, how are you sure that alert settings and thresholds are working fine in this mode?

The newcomers who do not use docker and will want to test out this module will have to enable UDP in their influxdb instance, most likely they will come to the support chats to complain about this.

I think we shoud run both the app and the tests in HTTP mode but we could run a subsets of the tests in UDP mode just to make sure we have some automated tests using the UDP functionality and confirming is working as expected.

I just tried the following configuration and I was able to run a subset of the tests with only a few failures after adding a sleep of 0.1 in the timeseries_write celery task:

[[udp]] enabled = true database = "openwisp2_test" batch-size = 1 batch-pending = 1 batch-timeout = "0.1s"

A possible subsets of the test to run is the following, please double check my suggestion to see if there's anything critical regarding the interaction with the timeseries DB to add:

./manage.py test openwisp_monitoring.device.tests.test_api openwisp_monitoring.device.tests.test_models openwisp_monitoring.monitoring.tests.test_monitoring_notifications openwisp_monitoring.monitoring.tests.test_db_creation openwisp_monitoring.monitoring.tests.test_models openwisp_monitoring.check.tests.test_ping openwisp_monitoring.check.tests.test_models

The main problem is that the logic which performs the check of thresholds and alert settings relies on the fact that the data is written immediately, so we have to look into ways to work around that.

openwisp_monitoring/monitoring/tests/__init__.py

nemesifier · 2023-01-24T17:54:32Z

openwisp_monitoring/monitoring/tests/test_monitoring_notifications.py

@@ -127,10 +128,12 @@ def test_general_check_threshold_crossed_for_long_time(self):
            self.assertEqual(m.is_healthy_tolerant, True)
            self.assertEqual(Notification.objects.count(), 0)

+        time.sleep(0.1)


make sure this sleep is executed only when in UDP mode and use the function mentioned above

nemesifier · 2023-01-26T14:06:23Z

tests/openwisp2/settings.py

@@ -31,7 +31,10 @@
    'OPTIONS': {'udp_writes': True, 'udp_port': 8089},


Do not default to UDP as that will cause issues with new developers trying this module out. Default on TCP and enable UDP in the tests.

nemesifier

Please also update the docs where it mentions the various timeseries retry options to make it explicit that it doesn't apply when using UDP mode.

I also see no mention of this new feature in the README and that's not good.

nemesifier

Let's test this more before merging.

nemesifier

Ok great one docs change requested below.

nemesifier · 2023-02-08T18:48:06Z

README.rst

+    enabled = true
+    bind-address = "127.0.0.1:8090"
+    database = "openwisp2"
+    retention-policy = 'short'


Please mention that when using ansible-openwisp2 this can be done easily by setting the new var addded in openwisp/ansible-ow-influxdb#17 to true and link the role variables section of the ansible-ow-influxdb repo for reference.
This should help users figure out how to use this feature autonomously.

…458 - Additional options can be specified to the TIMESERIES_DATABASE setting which are passed to the underlying backend library. This allows using UDP for writing to InfluxDB. - "openwisp_monitoring.db.backends.influxdb.client.DatabaseClient.write" will use "InfluxDBClient.write_points" instead of "InfluxDBClient.write" because the former allows writing data using the UDP protocol. Closes #458

nemesifier requested changes Dec 29, 2022

View reviewed changes

nemesifier changed the title ~~[feature] Allowed specifying additional options for InfluxDB backend #458~~ [feature] Allowed specifying additional options for InfluxDB backend (UDP) #458 Jan 19, 2023

pandafy force-pushed the issues/458-influxdb-udp branch 2 times, most recently from 022244a to 977144f Compare January 24, 2023 14:36

nemesifier reviewed Jan 24, 2023

View reviewed changes

pandafy force-pushed the issues/458-influxdb-udp branch from 3198e37 to a2ce4cb Compare January 25, 2023 04:15

nemesifier requested changes Jan 26, 2023

View reviewed changes

nemesifier requested changes Jan 27, 2023

View reviewed changes

nemesifier approved these changes Jan 30, 2023

View reviewed changes

pandafy force-pushed the issues/458-influxdb-udp branch from cfade98 to 21f084d Compare February 7, 2023 12:37

nemesifier requested changes Feb 8, 2023

View reviewed changes

pandafy added 11 commits February 9, 2023 13:54

[req-changes] Updated InfluxDB port

d26efd3

[chores] Added InfluxDB configuration for test database

e791b06

[tests] Run tests with InfluxDB in UDP mode

764c178

[chores] Updated CI

2f2a394

[tests] Fixed failing tests

3a7b0dc

[req-changes] Updated documentation

7ecc92d

[skipc ci] Fixed README code example

657e916

[feature] Fallback to TCP if data size exceeds UDP limit

aee9737

[skip ci][docs] Improved docs

707b598

[docs] Improved docs

fa195eb

pandafy force-pushed the issues/458-influxdb-udp branch from ece7394 to fa195eb Compare February 9, 2023 08:24

[skip ci] Fixed docs

3f6e529

pandafy force-pushed the issues/458-influxdb-udp branch from e4b34d4 to 3f6e529 Compare February 9, 2023 09:20

nemesifier approved these changes Feb 9, 2023

View reviewed changes

nemesifier merged commit 476e7a3 into master Feb 9, 2023

nemesifier deleted the issues/458-influxdb-udp branch February 9, 2023 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Allowed specifying additional options for InfluxDB backend (UDP) #458 #472

[feature] Allowed specifying additional options for InfluxDB backend (UDP) #458 #472

pandafy commented Dec 23, 2022

coveralls commented Dec 23, 2022 •

edited

Loading

pandafy commented Dec 29, 2022

nemesifier left a comment

nemesifier Dec 29, 2022

nemesifier Jan 24, 2023

nemesifier Jan 26, 2023

nemesifier left a comment

nemesifier left a comment

nemesifier left a comment

nemesifier Feb 8, 2023

		@@ -31,7 +31,10 @@
		'OPTIONS': {'udp_writes': True, 'udp_port': 8089},

[feature] Allowed specifying additional options for InfluxDB backend (UDP) #458 #472

[feature] Allowed specifying additional options for InfluxDB backend (UDP) #458 #472

Conversation

pandafy commented Dec 23, 2022

coveralls commented Dec 23, 2022 • edited Loading

pandafy commented Dec 29, 2022

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier Dec 29, 2022

Choose a reason for hiding this comment

nemesifier Jan 24, 2023

Choose a reason for hiding this comment

nemesifier Jan 26, 2023

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier Feb 8, 2023

Choose a reason for hiding this comment

coveralls commented Dec 23, 2022 •

edited

Loading