feat: improve error handling #61

NorbertNader · 2022-02-16T23:50:50Z

add status and type to error callback for time series data
add error handling in asset module
fix skiped tests
refactor subscribeToAssetTree to accept observer instead of next callback

Overview

Provide an explanation of what the change is

Tests

unit test

Legal

This project is available under the Apache 2.0 License.

NorbertNader · 2022-02-17T00:04:09Z

packages/core/src/data-module/data-cache/dataActions.ts

@@ -48,11 +49,15 @@ export interface ErrorResponse extends Action<'ERROR'> {
  payload: {
    id: DataStreamId;
    resolution: Resolution;
-    error: string;
+    error: string | SiteWiseErrorDetails;


You will see a lot of these string | SiteWiseErrorDetails in the data-module. The reason I did that is because currently synchrocharts expects a string, but really just checks if there is an error.

Is there anywhere we are still setting error to a string after your change here? Trying to understand how synchrocharts is forcing our hand on core data module types.

Honestly no, not in iot-app-kit though synchrocharts does expect a string. I think we might be able to but we heavily use import { DataPoint } from '@synchro-charts/core'; in the caching so reducers, actions etc. We could probably refactor this to not use synchrocharts, which probably makes sense if we want to make this extensible we should not couple the core with synchrocharts.

We shouldn't do anything because of synchrocharts unless there's a really good reason. I've already created a different DataStream type owned by IoT App Kit. If we want a different type than what is used in synchro charts, we should add some adapter logic within either the time-series-connector

The reality is we are doing a lot of things because of synchro-charts, e.g. in caching, forcing us to use this union. As an alternative we could refactor all instances where we use types directly from synchro-charts in core or change the error type in import { DataPoint } from '@synchro-charts/core'; from string to the same structure of ErrorDetails.

I like the idea of an adapter though, so something like sitewise-time-series-connector. We could extract themsg out from the error object.

Actually, here is my proposal. I just updated this PR with the error enforcing the ErrorDetails type. Second I would recommend updating the synchro-charts DataPoint error type to adhere to this structure and make use of the type and status.

NorbertNader · 2022-02-17T00:06:03Z

packages/components/src/components/iot-resource-explorer/sitewise-resource-explorer.tsx

@@ -97,7 +116,7 @@ export class SitewiseResourceExplorer {
        filterPlaceholder={filtering?.placeholder}
        onExpandChildren={this.expandNode}
        onSelectionChange={this.onSelectionChange}
-        empty={this.empty || this.defaults.empty}
+        empty={empty}


Was not able to test this with @stencil/core/testing, I want to backfill on these tests with cypress component testing as part of the next task of backfilling integration tests for components.

NorbertNader · 2022-02-17T00:09:43Z

packages/core/src/asset-modules/sitewise-asset-tree/assetTreeSession.ts

@@ -112,24 +131,31 @@ export class SiteWiseAssetTreeSession {

    // load related Asset Model and any of the requested properties that the Model contains
    if (this.query.withModels || this.query.propertyIds?.length) {


Didn't cover the error path with tests, will probably want to backfill once we do something with these errors.

NorbertNader · 2022-02-17T00:16:32Z

packages/core/src/data-module/data-cache/dataCacheWrapped.ts

 import { toDataStreams } from './toDataStreams';
-import { DataStream } from '../types';
+import { SiteWiseErrorDetails } from '../../asset-modules';


Hmm, maybe asset-modules isn't the best path for this and should rename to just ErrorDetails.

Agreed, with the way it's named so generally, are you planning on making this the default error format across all our current and future SiteWise modules? Perhaps it should go into top level iotsitewise types.

Yes, and agreed, will move this to top level.

NorbertNader · 2022-02-17T00:24:23Z

I honestly think there is still a lot of room for improvement in terms of error handling and UX but I think this is a good first take.

boweihan · 2022-02-17T17:40:48Z

packages/components/src/testing/testing-ground/siteWiseQueries.ts

@@ -1,12 +1,12 @@
 const STRING_ASSET_ID = 'f2f74fa8-625a-435f-b89c-d27b2d84f45b';
 const STRING_PROPERTY_ID = '797482e4-692f-45a2-b3db-17979481e9c3';

-export const DEMO_TURBINE_ASSET_1 = 'f2f74fa8-625a-435f-b89c-d27b2d84f45b';
+export const DEMO_TURBINE_ASSET_1 = '00eeb4b1-5017-48d4-9f39-1066f080a822';


Nooo, my poor assets IDs :)

Haha, yeah we probably want to clean this up before we go public.

boweihan · 2022-02-17T17:45:23Z

packages/core/src/asset-modules/sitewise-asset-tree/assetTreeSession.spec.ts

+
+  replayData.addErrors([error]);
+
+  it('When requesting root asset fails the error is returned', (done) => {


nit: test naming grammar! "it returns the error when requesting root asset fails"

Will update, thanks!

boweihan · 2022-02-17T17:53:25Z

packages/core/src/data-module/data-cache/dataCacheWrapped.ts

 import { toDataStreams } from './toDataStreams';
-import { DataStream } from '../types';
+import { SiteWiseErrorDetails } from '../../asset-modules';


Agreed, with the way it's named so generally, are you planning on making this the default error format across all our current and future SiteWise modules? Perhaps it should go into top level iotsitewise types.

boweihan · 2022-02-17T18:00:47Z

packages/core/src/data-module/data-cache/dataActions.ts

@@ -48,11 +49,15 @@ export interface ErrorResponse extends Action<'ERROR'> {
  payload: {
    id: DataStreamId;
    resolution: Resolution;
-    error: string;
+    error: string | SiteWiseErrorDetails;


Is there anywhere we are still setting error to a string after your change here? Trying to understand how synchrocharts is forcing our hand on core data module types.

boweihan · 2022-02-17T18:17:18Z

packages/core/src/asset-modules/sitewise/requestProcessor.ts

+        .then((result) => {
+          observer.next(result);
+        })
+        .catch((err) => {


I'm not an expert on observables, but it seems a little cumbersome to manually capture all these errors across observables that are already managed by parent classes - requestProcessorWorker and requestProcessorWorkerGroup. Do you think it's possible to use something like https://rxjs.dev/api/operators/catchError to implement a global handler for the request processor? If this is possible perhaps it's something we can also generalize across modules - it looks like it could also help us implement features like retries.

This is pretty interesting but I am not sure how we could implement this at a higher level. Each one of these api calls will have to catch individually but maybe I am missing something.

I don't think this would work the way we would want it to work https://stackblitz.com/edit/fbgiub?devtoolsheight=50

unfortunate, needing to listen for errors everywhere seems like a hassle, something to tackle in the future

diehbria · 2022-02-18T04:11:51Z

packages/core/src/asset-modules/sitewise-asset-tree/assetTreeSession.spec.ts

+  });
+
+  it('When requesting asset model fails the error is returned', (done) => {
+    const session: SiteWiseAssetTreeSession = new SiteWiseAssetTreeSession(new MockSiteWiseAssetSession(replayData), {


should close the session at the end of the test, so that we don't have open handles

fair point.

* add status to error callback for time series data * add error handling in asset module * fix skiped tests * refactor subscribeToAssetTree to accept observer instead of next callback

See all commit messages below Fixes the nested camera view locations for activeCameraSettings (#102) * Fixes the nested camera view locations for activeCameraSettings The problem is that the node.transform is not in world coordinates. To accurately represent this we create the transform from the actualy object3D world position, rotation, and scale * Clean up based on feedback * Rebase needed Fix target calculation for Camera settings (#104) Have the Camera Preview pull from the selected node rather than using active camera settings (#101) Adjusting the active Camera settings also affected the main camera's settings and was a bug. Utilizing the camera component on the selected node allowed for the same render without the issue as the active Camera settings aren't necessarily set to active by editing anyway Update the camera component inspector editor to match the UX (#99) Visual restructuring Bug fixes Code clean up Adds a CameraPreview component (#97) This performs a LateRender to a smaller viewport designated by the tracked div element When a camera is selected the div and the render of the camera's view will appear when in edit mode Adds the activeCamera property (#66) Based on this property we set the active camera unless we have a selected data binding as that should take precedence Make TopBar always visible and Add Camera list (#64) Clicking a camera name in the list will set that camera as the active camera in either editing or viewing Add an incremental count to the Camera object to give unique names since creation Revert "Add activeCamera Property to Scene Viewer (#56)" (#61) This reverts commit 98bce138a4bf572b6f25562c458ace8d7ac3f560. Add activeCamera Property to Scene Viewer (#56) State setup seems to appear incorrectly in storybook such that the Camera View is not utlizing the activeCamera unless the store has already been created. This is something that will need to be looked into further. Add Set from current view, button to inspector (#52) Also refactors how we set the object from the current view Fixes the bug where we only support one parent for rotational transform Add basic Inspector Editor for the Camera Component (#49) Change Camera creation mode to View Camera (From current) as in design (#46) Update the CameraComponent to be creatable and selectable (#45) This introduces the ability to set a globally stored active camera which can alter the settings and viewing location of the main camera. These are less cameras themselves than editor and data representations

* add status to error callback for time series data * add error handling in asset module * fix skiped tests * refactor subscribeToAssetTree to accept observer instead of next callback Co-authored-by: Norbert Nader <nnader@amazon.com>

NorbertNader requested review from boweihan, diehbria and gareth-amazon February 16, 2022 23:50

NorbertNader commented Feb 17, 2022

View reviewed changes

boweihan reviewed Feb 17, 2022

View reviewed changes

NorbertNader force-pushed the asset-module-error-handling branch 4 times, most recently from d840db4 to 7e41599 Compare February 17, 2022 20:49

NorbertNader requested a review from boweihan February 17, 2022 21:20

NorbertNader force-pushed the asset-module-error-handling branch from 7e41599 to 7d88c01 Compare February 17, 2022 21:25

diehbria reviewed Feb 18, 2022

View reviewed changes

NorbertNader force-pushed the asset-module-error-handling branch 3 times, most recently from 3c0393d to c0b5c13 Compare February 18, 2022 06:54

NorbertNader requested a review from diehbria February 18, 2022 06:55

NorbertNader force-pushed the asset-module-error-handling branch from c0b5c13 to 215875a Compare February 18, 2022 07:07

feat: improve error handling

2c0c6ba

* add status to error callback for time series data * add error handling in asset module * fix skiped tests * refactor subscribeToAssetTree to accept observer instead of next callback

NorbertNader force-pushed the asset-module-error-handling branch from 215875a to 2c0c6ba Compare February 18, 2022 07:18

NorbertNader requested a review from fpauer February 18, 2022 19:29

boweihan approved these changes Feb 18, 2022

View reviewed changes

NorbertNader merged commit 183c01d into main Feb 18, 2022

NorbertNader deleted the asset-module-error-handling branch February 22, 2022 17:58

github-actions bot mentioned this pull request Oct 26, 2022

chore(main): release 1.0.0 #296

Closed

This was referenced Nov 2, 2022

chore: release 3.0.0 #313

Closed

chore: release 3.0.0 #314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve error handling #61

feat: improve error handling #61

NorbertNader commented Feb 16, 2022 •

edited

Loading

NorbertNader Feb 17, 2022

boweihan Feb 17, 2022

NorbertNader Feb 17, 2022 •

edited

Loading

diehbria Feb 18, 2022

NorbertNader Feb 18, 2022 •

edited

Loading

NorbertNader Feb 18, 2022 •

edited

Loading

NorbertNader Feb 17, 2022

NorbertNader Feb 17, 2022

NorbertNader Feb 17, 2022

boweihan Feb 17, 2022

NorbertNader Feb 17, 2022

NorbertNader commented Feb 17, 2022

boweihan Feb 17, 2022

NorbertNader Feb 17, 2022

boweihan Feb 17, 2022

NorbertNader Feb 17, 2022

boweihan Feb 17, 2022

boweihan Feb 17, 2022

boweihan Feb 17, 2022

NorbertNader Feb 17, 2022 •

edited

Loading

NorbertNader Feb 17, 2022

boweihan Feb 18, 2022

diehbria Feb 18, 2022

NorbertNader Feb 18, 2022

		@@ -112,24 +131,31 @@ export class SiteWiseAssetTreeSession {

		// load related Asset Model and any of the requested properties that the Model contains
		if (this.query.withModels \|\| this.query.propertyIds?.length) {


		replayData.addErrors([error]);

		it('When requesting root asset fails the error is returned', (done) => {

feat: improve error handling #61

feat: improve error handling #61

Conversation

NorbertNader commented Feb 16, 2022 • edited Loading

Overview

Tests

Legal

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NorbertNader Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NorbertNader Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

NorbertNader Feb 18, 2022 • edited Loading

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

NorbertNader commented Feb 17, 2022

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

NorbertNader Feb 17, 2022 • edited Loading

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

NorbertNader commented Feb 16, 2022 •

edited

Loading

NorbertNader Feb 17, 2022 •

edited

Loading

NorbertNader Feb 18, 2022 •

edited

Loading

NorbertNader Feb 18, 2022 •

edited

Loading

NorbertNader Feb 17, 2022 •

edited

Loading