(analytics): Backend updates for Application dashboard CRUD v2 #2889
Conversation
Signed-off-by: ishangupta-ds <ishan@chaosnative.com>
Signed-off-by: ishangupta-ds <ishan@chaosnative.com>
Signed-off-by: ishangupta-ds <ishan@chaosnative.com>
Signed-off-by: ishangupta-ds <ishan@chaosnative.com>
ishangupta-ds
requested review from
whitetiger1399,
imrajdas,
arkajyotiMukherjee and
SarthakJain26
Signed-off-by: ishangupta-ds <ishan@chaosnative.com>
| @@ -395,11 +395,11 @@ type Mutation { | |||
| # Analytics | |||
| createDataSource(datasource: DSInput): DSResponse @authorized | |||
|
|
|||
| createDashBoard(dashboard: createDBInput): String! @authorized | |||
| createDashBoard(dashboard: createDBInput): listDashboardReponse! @authorized | |||
There was a problem hiding this comment.
Typo: listDashboardReponse -> listDashboardResponse
There was a problem hiding this comment.
Sure updating it, it may have been present from a long time.
| timestamp := strconv.FormatInt(time.Now().Unix(), 10) | ||
|
|
||
| if dashboard.DbID == "" && dashboard.DsID == "" { | ||
| return "", errors.New("DashBoard ID or Datasource is nil or empty") | ||
| return "could not find the dashboard or the connected data source", errors.New("dashBoard ID or data source ID is nil or empty") |
There was a problem hiding this comment.
error should be or logic is and
There was a problem hiding this comment.
any one or both can be false
There was a problem hiding this comment.
You have given && in the if condition, so this error message will be returned only if both are not present
There was a problem hiding this comment.
yes, thats likely, will update it
There was a problem hiding this comment.
updated the condition @SarthakJain26
|
|
||
| existingDashboard, err := dbOperationsAnalytics.GetDashboard(query) | ||
| if err != nil { | ||
| return "error fetching dashboard details", fmt.Errorf("error on query from dashboard collection by projectid: %v", err) |
There was a problem hiding this comment.
Instead of returning a custom error message(for the err part) every time, we can just return err. User can understand the error by the string that is being returned. (Just a suggestion)
There was a problem hiding this comment.
yes but the problem then is to return an empty string since on success we are returning a text message it may be a good practice to return the sting and as for the custom part, the error returned may be a database error and we don't want the user to know the specifics of the database error over the graphQL query (it might be better to use a custom message which may be more understandable to the end user and also for logging purposes) its not a panic.
There was a problem hiding this comment.
We are returning two parameters: string and error, I'm talking about the error part. Instead of adding a custom error message, we can only return err, that we are doing now as well.
I'm just suggesting to replace fmt.Errorf("error on query from dashboard collection by projectid: %v", err) with err
There was a problem hiding this comment.
Yes as i mentioned that may spill the collection name etc if its a database error or something. custom message will let us control the logging (also sometimes error returned span a lot of lines and corrupt the server logs) it may be better this way cc: @rajdas98 @gdsoumya There had been a similar case where no data in collection user or something was being logged continuously sometime back.
There was a problem hiding this comment.
It's fine if you keep it this way, but you are printing err now as well.
| } | ||
| panels, err := dbOperationsAnalytics.ListPanel(query) | ||
| if err != nil { | ||
| return "error fetching panels", fmt.Errorf("error on querying from promquery collection: %v", err) |
There was a problem hiding this comment.
Have addressed that comment. Please let me know on your views on the reason of the custom error.
Signed-off-by: ishangupta-ds <ishan@chaosnative.com>
|
@SarthakJain26 the changes have been made PTAL again cc: @rajdas98 |
Signed-off-by: ishangupta-ds <ishan@chaosnative.com>
Proposed changes
Types of changes
Checklist
Special notes for your reviewer:
App. dashboard CRUD v2 PR #1