Add `sensor_value` function that returns latest sensor reading #34

SKAJohanVenter · 2018-07-11T13:04:07Z

Added function sensor_value to katportalclient that gets the latest value of a sensor (instance of SensorSample or SensorSampleValueTs ) via katmonitor.
Added tests
Added usage example script

JIRA: CB-3160
Issue: #33

Added function `sensor_value` to katportalclient that gets the latest value of a sensor via katmonitor. Added usage example script. JIRA: CB-3160

SKAJohanVenter · 2018-07-11T13:09:42Z

This PR depends on https://github.com/ska-sa/katportal/pull/333

EDIT: The above PR has been merged.

tockards

LGTM.

ajoubertza

Very nice, with good tests. Some minor things to change.

ajoubertza · 2018-07-11T13:37:48Z

examples/get_latest_sensor_value.py

+    sensor_names = yield portal_client.sensor_names(args.sensors)
+    print "\nMatching sensor names for pattern {}: {}".format(args.sensors, sensor_names)
+
+    # Fetch the details for the sensors found.


s/details/readings

ajoubertza · 2018-07-11T14:12:55Z

examples/get_latest_sensor_value.py

+            try:
+                sensor_value = yield portal_client.sensor_value(sensor_name,
+                                                                include_value_ts=True)
+            except SensorNotFoundError, exc:


Suggest the new-style syntax here:
except SensorNotFoundError as exc:

ajoubertza · 2018-07-11T15:08:58Z

katportalclient/test/test_client.py

+
+        self.mock_http_async_client().fetch.side_effect = fake_http_response(mon_response)
+        result = yield self._portal_client.sensor_value("anc_tfr_m018_l_band_offset")
+        expected_result = SensorSample(timestamp=1531302437.772357, value=43680.0,


This many digits in the timestamp is at the limit of what a double floating point variable can store, so it ends up slightly different to the string in the mon_response. Suggest rounding off all the times to the nearest second to avoid this difference and make it easier to read.

ajoubertza · 2018-07-11T15:09:53Z

katportalclient/test/test_client.py

+        """Test that we can handle single result"""
+        mon_response = ('[{"status":"nominal",'
+                        '"name":"some_other_sample","component":"anc","value":43680.0,'
+                        '"value_ts":111.111,"time":111.111}]')


Rather use different values for value_ts and time. Then we can ensure the fields weren't swapped by accident.

ajoubertza · 2018-07-11T15:10:44Z

katportalclient/test/test_client.py

+
+
+def fake_http_response(response_string):
+    """Used as a mock side effect response for AsyncHTTPClient.fetch"""


This is a nice solution.

ewanbarr · 2018-07-12T09:43:23Z

The example script currently pops an error when being run. For example:

$ python examples/get_latest_sensor_value.py --host portal.mkat.karoo.kat.ac.za m009_observer
Matching sensor names for pattern ['m009_observer']: [u'm009_observer']
Traceback (most recent call last):
  File "examples/get_latest_sensor_value.py", line 74, in <module>
    io_loop.run_sync(main)
  File "/Users/ebarr/anaconda/envs/py27/lib/python2.7/site-packages/tornado/ioloop.py", line 458, in run_sync
    return future_cell[0].result()
  File "/Users/ebarr/anaconda/envs/py27/lib/python2.7/site-packages/tornado/concurrent.py", line 238, in result
    raise_exc_info(self._exc_info)
  File "/Users/ebarr/anaconda/envs/py27/lib/python2.7/site-packages/tornado/gen.py", line 1063, in run
    yielded = self.gen.throw(*exc_info)
  File "examples/get_latest_sensor_value.py", line 40, in main
    include_value_ts=True)
  File "/Users/ebarr/anaconda/envs/py27/lib/python2.7/site-packages/tornado/gen.py", line 1055, in run
    value = future.result()
  File "/Users/ebarr/anaconda/envs/py27/lib/python2.7/site-packages/tornado/concurrent.py", line 238, in result
    raise_exc_info(self._exc_info)
  File "/Users/ebarr/anaconda/envs/py27/lib/python2.7/site-packages/tornado/gen.py", line 307, in wrapper
    yielded = next(result)
  File "/Users/ebarr/Soft/katportalclient/katportalclient/client.py", line 1245, in sensor_value
    url = self.sitemap['monitor'] + '/list-sensors/all'
KeyError: 'monitor'

SKAJohanVenter · 2018-07-12T09:46:16Z

@ewanbarr
This is expected, this change depends on a change in katportal (it also needs to be deployed), see my earlier comment:
#34 (comment)

ewanbarr · 2018-07-12T09:58:28Z

Ok, I can't test out this change with the portal server I have access to. Is there a development portal that is up that includes ska-sa/katportal#333 (I don't have permission to see this repo)?

Apart from that the interface looks fine.

SKAJohanVenter · 2018-07-12T10:03:41Z

@ewanbarr I'll email you an address you can use.

ajoubertza

Good to go 👍

lvdheever

Looks fine

lvdheever · 2018-07-12T11:34:57Z

examples/get_latest_sensor_value.py

+                sensor_value = yield portal_client.sensor_value(sensor_name,
+                                                                include_value_ts=True)
+            except SensorNotFoundError as exc:
+                print "\n", exc


Not sure what the "exc" looks like by it may have been good to include the sensor_name in this print (although it probably is in the exception). No need to change just this, though.

Yes, the sensor name is in the string, see https://github.com/ska-sa/katportalclient/pull/34/files#diff-6355b287a79bb44c686cf1256475bf01R1256

lvdheever · 2018-07-12T11:36:39Z

katportalclient/client.py

@@ -1213,6 +1213,78 @@ def sensor_detail(self, sensor_name):
        else:
            raise tornado.gen.Return(results[0])

+    @tornado.gen.coroutine
+    def sensor_value(self, sensor_name, include_value_ts=False):
+        """Return the latest value of a sensor.


Don't need to change. Not sure if "sensor_value" is a bit confusing as it actually returns the latest sensor sample - which include the value. But I think it is fine as the example shows how it is used. Could have been something like "sensor_reading" or "get_latest" ?

lvdheever · 2018-07-12T11:37:12Z

katportalclient/test/test_client.py

+        res = yield self._portal_client.sensor_value("anc_tfr_m018_l_band_offset_average",
+                                                     include_value_ts=True)
+        assert res == expected_result
+


Good set of tests

SKAJohanVenter added 3 commits July 10, 2018 16:03

Added function sensor_value and usage example.

57fa4d9

Added function `sensor_value` to katportalclient that gets the latest value of a sensor via katmonitor. Added usage example script. JIRA: CB-3160

Added tests and handling non JSON requests from katmonitor

e8734f8

Grammar fixes. Extra documentation

02c312c

SKAJohanVenter requested review from lvdheever and ajoubertza July 11, 2018 13:04

SKAJohanVenter added the do not merge yet label Jul 11, 2018

ajoubertza requested a review from tockards July 11, 2018 13:33

SKAJohanVenter removed the do not merge yet label Jul 12, 2018

tockards approved these changes Jul 12, 2018

View reviewed changes

SKAJohanVenter mentioned this pull request Jul 12, 2018

Feature to return latest sensor value #33

Closed

ajoubertza suggested changes Jul 12, 2018

View reviewed changes

Minor changes from PR feedback

610bc9c

ajoubertza approved these changes Jul 12, 2018

View reviewed changes

lvdheever approved these changes Jul 12, 2018

View reviewed changes

SKAJohanVenter merged commit 6a437d7 into master Jul 12, 2018

SKAJohanVenter deleted the user/johan/CB-3160/get_latest_sensor_value branch July 12, 2018 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `sensor_value` function that returns latest sensor reading #34

Add `sensor_value` function that returns latest sensor reading #34

SKAJohanVenter commented Jul 11, 2018 •

edited by ajoubertza

SKAJohanVenter commented Jul 11, 2018 •

edited

tockards left a comment

ajoubertza left a comment

ajoubertza Jul 11, 2018

ajoubertza Jul 11, 2018

ajoubertza Jul 11, 2018

ajoubertza Jul 11, 2018

SKAJohanVenter Jul 12, 2018

ajoubertza Jul 11, 2018

ewanbarr commented Jul 12, 2018

SKAJohanVenter commented Jul 12, 2018

ewanbarr commented Jul 12, 2018

SKAJohanVenter commented Jul 12, 2018

ajoubertza left a comment

lvdheever left a comment

lvdheever Jul 12, 2018

SKAJohanVenter Jul 12, 2018

lvdheever Jul 12, 2018 •

edited

lvdheever Jul 12, 2018



		def fake_http_response(response_string):
		"""Used as a mock side effect response for AsyncHTTPClient.fetch"""

Add sensor_value function that returns latest sensor reading #34

Add sensor_value function that returns latest sensor reading #34

Conversation

SKAJohanVenter commented Jul 11, 2018 • edited by ajoubertza

SKAJohanVenter commented Jul 11, 2018 • edited

tockards left a comment

Choose a reason for hiding this comment

ajoubertza left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewanbarr commented Jul 12, 2018

SKAJohanVenter commented Jul 12, 2018

ewanbarr commented Jul 12, 2018

SKAJohanVenter commented Jul 12, 2018

ajoubertza left a comment

Choose a reason for hiding this comment

lvdheever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvdheever Jul 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `sensor_value` function that returns latest sensor reading #34

Add `sensor_value` function that returns latest sensor reading #34

SKAJohanVenter commented Jul 11, 2018 •

edited by ajoubertza

SKAJohanVenter commented Jul 11, 2018 •

edited

lvdheever Jul 12, 2018 •

edited