vapor-ware / synse-server

An HTTP API for monitoring and controlling physical and virtual devices
https://synse.readthedocs.io/en/latest/server/intro/
GNU General Public License v3.0
39 stars 11 forks source link

unify responses for 'data not found' from plugin #88

Closed edaniszewski closed 6 years ago

edaniszewski commented 6 years ago

Description

It seems like when a device does not have any data (e.g. the plugin SDK fails to read anything off of a configured device) we either:

The behavior here should be unified so we always expect the same thing for the same class of error. I'm not sure what conditions make it so that "null" is returned vs. a 500, so that will need to be investigated first.

To recreate this, there can be a device configured in a plugin that doesn't physically exist, so the SDK can't read off of it. Then read that device off of Synse Server.

I think that returning "null" (or the actual null type) should be fine, but I'd like to hear what others think -- does it seem like returning null is better, or returning a 500 error when there are no readings for a configured device.

Note that this case of 'no readings' can occur when:

timfallmk commented 6 years ago

I would vote that we not use 500 in that that should be reserved for internal server-side errors that aren't resolvable into a coherent message. We should probably only be sending that in the case that things are really fuckered and we can't figure out why.

Could we throw a 404 for not found/not reachable? If not present in the config, I'd like to see that as an error. Any failures in the process should be reported if categorizable, and if not then 500 as a last resort.

edaniszewski commented 6 years ago

the above makes sense and is easy enough.

i wrote a quick script that polls all thermistors we have configured to see which ones get readings, which return 500, which return null

$ python poll.py 10.193.1.6
                                 |         chamber |  vec1-c1-austin |  vec6-c1-austin |  vec4-c1-austin |  vec5-c1-austin |  vec3-c1-austin |  vec2-c1-austin | 
-------------------------------- | --------------- | --------------- | --------------- | --------------- | --------------- | --------------- | --------------- | 
3f336eb0bb188de8c72650d406225ffa |            None |             500 |             500 |             500 |             500 |             500 |             500 | 
80de4a2b73c41cf71ef7d30e1f2b9494 |            None |           21.94 |           21.94 |           21.23 |           21.86 |           21.39 |           21.31 | 
b4bf920c24ad7c2e23360ffcda9f4b5d |            None |            null |            null |            null |            null |            null |            null | 
b0efdac41dfc7e8261dfd4bd2369b161 |            None |           22.64 |           22.64 |            21.7 |           22.41 |           21.62 |           22.02 | 
fa941bad2325cb097deb97743007ff36 |            None |           21.47 |           21.62 |           20.68 |           21.47 |           20.61 |           21.08 | 
2079f261767869c929e01ba10132eceb |            None |           21.39 |           21.86 |            21.0 |           21.23 |           20.76 |           21.23 | 
18185208cbc0e5a4700badd6e39bb12d |            None |           22.25 |           22.17 |           22.33 |           22.49 |           22.33 |           22.25 | 
5beec8c6db5445fd164566478997ff30 |            None |           21.62 |           21.94 |           20.76 |            21.7 |           20.61 |           21.31 | 
1e93da83dd383757474f539314446c3d |            None |           21.39 |           21.94 |           20.45 |           21.39 |           20.37 |            21.0 | 
e955c02a134ff2ac6e9e3a9dedc67dc1 |            None |           21.39 |           21.86 |           21.31 |           21.47 |           21.08 |           21.39 | 
c5d8272bc1106aa12225850d33483f33 |            None |            null |            null |            null |            null |            null |            null | 
645a790d97fc93ee12ec5fbb514230d7 |            None |           21.39 |           21.62 |           20.29 |            21.0 |           20.53 |           20.84 | 

checking the device info for the 500 response device

$ curl 10.193.1.6:5000/synse/2.0/info/vec1-c1-austin/vec/3f336eb0bb188de8c72650d406225ffa
{
  "timestamp":"2018-03-13T03:09:01.692682571Z",
  "uid":"3f336eb0bb188de8c72650d406225ffa",
  "type":"temperature",
  "model":"MAX11610",
  "manufacturer":"Maxim Integrated",
  "protocol":"i2c",
  "info":"Rack Temperature 0 Top Left",
  "comment":"",
  "location":{
    "rack":"vec1-c1-austin",
    "board":"vec"
  },
  "output":[
    {
      "type":"temperature",
      "data_type":"",
      "precision":2,
      "unit":{
        "name":"degrees celsius",
        "symbol":"C"
      },
      "range":{
        "min":0,
        "max":100
      }
    }
  ]
}

and checking the device info for a "null" response device

$ curl 10.193.1.6:5000/synse/2.0/info/vec1-c1-austin/vec/b4bf920c24ad7c2e23360ffcda9f4b5d
{
  "timestamp":"2018-03-13T03:09:31.624861759Z",
  "uid":"b4bf920c24ad7c2e23360ffcda9f4b5d",
  "type":"temperature",
  "model":"MAX11610",
  "manufacturer":"Maxim Integrated",
  "protocol":"i2c",
  "info":"Rack Temperature 8 Top Right",
  "comment":"",
  "location":{
    "rack":"vec1-c1-austin",
    "board":"vec"
  },
  "output":[
    {
      "type":"temperature",
      "data_type":"",
      "precision":2,
      "unit":{
        "name":"degrees celsius",
        "symbol":"C"
      },
      "range":{
        "min":0,
        "max":100
      }
    }
  ]
}

there is no difference in the device info here, so will need to dig more.