Implement JSON string parsing to extract anomaly-related fields for property query type #644
Conversation
…roperty query type
hiralpatelHM
requested review from
iwysiu and
njvrzm
and removed request for
a team
iwysiu
left a comment
iwysiu
left a comment
There was a problem hiding this comment.
Hi! I did an initial pass of this, but do you have links to any aws docs or anything for more context on this? I feel like I don't have the full story on what this is implementing.
Also the code in ParseJsonFields specifically could be more commented since its doing a lot of logic and not all of it's immediately clear, for example where the diag_anomaly field get added.
| return propertyNameMap | ||
| } | ||
|
|
||
| func isRequireJSONParsing(query models.BaseQuery) bool { |
There was a problem hiding this comment.
nit: this could be requiresJsonParsing instead
| propertyNameMap := map[string]string{} | ||
| if assetID == "" { | ||
| backend.Logger.Warn("BuildPropertyNameMap: empty assetId") | ||
| return propertyNameMap |
There was a problem hiding this comment.
Instead of returning an empty map, we could return an error instead, and either log the error in ParseJsonFields or exit early if that's appropriate
|
|
||
| var obj map[string]interface{} | ||
| if err := json.Unmarshal([]byte(rawStr), &obj); err != nil { | ||
| backend.Logger.Warn("ParseJSONFields: corrupted JSON, skipping row", "err", err, "frame", frame.Name, "row", r) |
There was a problem hiding this comment.
This log should include the field. Also I know you have a test for it but I would think users would expect an error in this situation instead of partially returned data. Do we expect to get corrupted JSON returned for this type of query relatively often?
| // Required fields check | ||
| for _, req := range []string{"timestamp", "prediction", "prediction_reason"} { | ||
| if _, ok := obj[req]; !ok { | ||
| backend.Logger.Warn("ParseJSONFields: missing required field", "field", req, "frame", frame.Name, "row", r) |
There was a problem hiding this comment.
this log should include the field. The same thing where if these are required fields, I feel like users would expect an error
| } | ||
| jsonParsed = true | ||
| // Required fields check | ||
| for _, req := range []string{"timestamp", "prediction", "prediction_reason"} { |
There was a problem hiding this comment.
The required fields should be consts since we use them multiple times
| } | ||
|
|
||
| for key, val := range obj { | ||
| if key == "diagnostics" || key == "timestamp" { |
There was a problem hiding this comment.
shouldn't we also skip anomaly score if we handle it in the next block? Also are we intentionally skipping timestamp?
| if !ok { | ||
| continue | ||
| } | ||
| rawName, _ := diagObj["name"].(string) |
There was a problem hiding this comment.
We should probably have a check for if name exists in diagObj
2 participants
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #Anomaly Detection
Special notes for your reviewer: