Closed edaniszewski closed 6 years ago
Should this one be fixed at the Synse Server level? It is said in the Synse Server documentation that
If a read is not supported, an error will be returned with the JSON response specifying the cause as reads not permitted.
Because the emulator plugin doesn't have a non-readable device like the example above, I can't simulate an error yet. However, when I test it with a POST request with a mistype device name that should have a similar error response from the server, the result is what I expect:
Synse Server responded with error:
{
"http_code": 404,
"error_id": 4000,
"description": "device not found",
"timestamp": "2018-06-20T19:40:36.637196Z",
"context": "rack-1/vec/f52d29fecf05a195af13f14c7306cfed0 does not correspond with a known device"
}
In the Synse CLI, we create a client to make request and use a helper function to check and wrap the response error. If the validation happens at the Synse Server level like that POST request, then we don't have to change anything because this should automatically work.
hmmm. that sounds reasonable. I recreated this with the AMT plugin
edaniszewski ~ ➜ curl localhost:5000/synse/2.1/read/rack-1/vec/ecbb145965fd5e86fa7dee495ded6cab
{
"kind":"boot_target",
"data":{
}
}
edaniszewski ~ ➜ synse server read rack-1 vec ecbb145965fd5e86fa7dee495ded6cab
TYPE VALUE UNIT TIMESTAMP
If definitely doesn't feel like the right behavior from Synse Server, but I think it works this way because upstream stuff (namely synse-graphql) didn't handle read errors well at all, so a patch was made in synse server temporarily. I think you are right and the patch should be backed out.
I think that maybe the work @Kontazler did today in synse-graphql might fix the issue with synse-graphql getting no readings? (https://github.com/vapor-ware/synse-graphql/issues/13) -- or is that something else?
The fix for synse-grapqhl was specifically for the prometheus metric creation. It executes GraphQL queries against synse-grapqhl's created schema, which is based on scans and reads. Synse-Grapqhql appears to check for "null"
values as the result of /read
requests, here in Device resolve_detail. I don't know if Synse Server was ever configured to return "null"
for any reason. But if the value isn't "null"
, it either exists or is passed as None
. The change this morning was to handle None
as a reading, specifically when Prometheus accesses that value.
Ah. So I guess that means it wasn't fixed.
The whole handling of "null" was the aforementioned patch that was made. Basically instead of failing because of no device (because I guess otherwise it would throw an error and synse-graphql didn't like that/surfaced that error as indistinguishable from other kinds of errors, so it polluted the metrics?).
I think there needs to be a work item in synse-graphql to do better about handling errors. Once thats in place, then Synse Server can fail like it should, and then this issue should be a no-op, as @hoanhan101 suggested.
I just tested it with the Sandbox Plugin and it then returned the error from Synse Server as we expected.
➜ synse-cli git:(sdk1.0-updates) ./build/synse server scan
RACK BOARD DEVICE INFO TYPE
rack-1 vec 37ef30c3b62c0d9198c6028ddc58606d Reads data successfully ok
rack-1 vec 9e67acdb55b4ab96f558f49b932efd5b Always fails writing error
rack-1 vec bc2d61fcbe145ca4a58a6a0691c47f70 Writes data successfully ok
rack-1 vec c18e2e92f86e6e240586ed551c31e4b2 Always fails reading error
➜ synse-cli git:(sdk1.0-updates) ./build/synse server read rack-1 vec 9e67acdb55b4ab96f558f49b932efd5b
Synse Server responded with error:
{
"http_code": 500,
"error_id": 5001,
"description": "failed read command",
"timestamp": "2018-07-11T13:40:00.723895Z",
"context": "\u003c_Rendezvous of RPC that terminated with (StatusCode.UNKNOWN, reading not enabled for device rack-1-vec-9e67acdb55b4ab96f558f49b932efd5b (no read handler))\u003e"
}
➜ synse-cli git:(sdk1.0-updates) ./build/synse plugin --tcp localhost:5001 read rack-1 vec 9e67acdb55b4ab96f558f49b932efd5b
rpc error: code = Unknown desc = reading not enabled for device rack-1-vec-9e67acdb55b4ab96f558f49b932efd5b (no read handler)
neat! I guess that means we can call this done, i'll let you do the honors of closing (:
if a device does not support reading and we try reading from it, we see and empty read response:
In this case, I think we'd want an error that says the device doesn't support reads.