feat(data-point): Add reducer stack when reducers throw errors #230

raingerber · 2018-02-23T21:36:49Z

What: closes #149 and closes #160 . I opened a previous PR for this issue, but that implementation was (probably) less efficient than this one.

Why: Helps with debugging

How: Add _stack and _input properties to error objects. It doesn't log anything though - leaving that to the user.

Instead of trying to create a tree data structure, this relies on the fact that resolveReducer is called recursively. That function generates a promise with a catch handler that re-throws the error, so the handler is called for every reducer in a given chain. So, if we have a reducer chain a -> b -> c that throws an error for c, each reducer has a chance to add data to the error object:

This is the sequence for that chain:

c -> creates a stack array: error._stack = ['c']
b -> updates the array: error._stack = ['b', 'c']
a -> updates the array: error._stack = ['a', 'b', 'c']

This means the final error object has a _stack property that records an id for each reducer in the chain that failed. That's the basic idea anyway -- resolveReducer also now takes a key parameter to add extra metadata about the reducer that's being resolved. For example, if we're resolving a ReducerMap on a given array, we need to know what index of the array was being resolved when the reducer failed. In that example, the key parameter would be the current index of the input array, and it would be added to the error._stack array.

NOTE: The stack traces do not working correctly for entities with the [] notation (e.g. hash:thing[]); specifically, it does not indicate what index of the array was being evaluated when the error was thrown. This would be fixed if we do #130.

Checklist:

Has Breaking changes
Documentation
Tests
Ready to be merged

…lue method (ViacomInc#222)

coveralls · 2018-02-23T21:40:01Z

Coverage remained the same at 100.0% when pulling 4adaea5 on raingerber:reducer-stack-second-attempt into cd77019 on ViacomInc:master.

raingerber · 2018-02-23T22:12:14Z

packages/data-point/lib/reducer-types/resolve.js

+  const value = accumulator.value
+  const result = Promise.try(() => getResolveFunction(reducer))
+    // NOTE: recursive call
+    .then(resolve => resolve(manager, resolveReducer, accumulator, reducer))


wrapping getResolveFunction so that resolveReducer will always return a promise, and errors thrown from this function will always have the _stack property

raingerber · 2018-02-23T22:28:35Z

packages/data-point/lib/entity-types/entity-request/resolve.js

+function resolveRequest (accumulator, resolveReducer) {
+  inspect(accumulator)
+  const acc = utils.set(accumulator, 'value', accumulator.options)
+  return resolveReducer(acc, requestReducer)


This gets rid of all the custom properties that were added to the error object here. My reasoning for this change is that I'm now adding error._input and error._stack properties that have a lot of the same information, and most of the other properties are just repeating data that's already available in the original error. Now that we're no longer doing console.info with the error message (#210), it seems less important to have this code.

raingerber · 2018-02-23T22:44:48Z

packages/data-point/README.md

+      error._value,
+      'foo'
+    )
+  })


wondering if this property should be _input instead of _value...

I'm thinking the same thing. input better matches what this actually is (the input provided to the failing reducer). The description you wrote supports that idea too:

When a reducer throws an error, DataPoint adds two properties to the error object: `_value:*` - the input value to the reducer that failed

raingerber · 2018-02-23T23:10:20Z

packages/data-point/lib/entity-types/base-entity/resolve.js

@@ -23,7 +23,8 @@ function resolveErrorReducers (manager, error, accumulator, resolveReducer) {
  const reducerResolved = resolveReducer(
    manager,
    errorAccumulator,
-    errorReducer
+    errorReducer,
+    [['error']]


the nested array forces stringifyReducerStack to format the output string in a particular way, and I'm using this approach for most of the entity lifecycle reducers. Don't really like it though...

Assuming this is kept it might be worth adding a comment about why it's implemented this way.

This allows for easier reducer stack traces

paulmolluzzo

This is awesome! And the new tests you wrote for the stack tracing are 💯! 👏 👏 👏

Left a few comments, and I believe most are questions not requests. I would like to have some comment in there about that nested array being used. The other comments I left could just be discussed first.

Fantastic work!

paulmolluzzo · 2018-02-28T14:02:13Z

packages/data-point/README.md

+
+When a reducer throws an error, DataPoint adds two properties to the error object:
+
+`_value:*` - the input value to the reducer that failed


Re-reading this, changing this to _input makes more sense.

paulmolluzzo · 2018-02-28T14:04:41Z

packages/data-point/lib/debug-utils/stack.test.js

+      expect(result).toHaveProperty('_value')
+      expect(result).toHaveProperty('_stack')
+      expect(result._value).toMatchSnapshot()
+      expect(result._stack).toMatchSnapshot()


Maybe we want to use toThrowErrorMatchingSnapshot()?

Does that work with promises?

No, it doesn't. ☹️

We can do this though, which should write the whole object:

async function testError (reducer, input) { await expect(dataPoint.resolve(reducer, input)).rejects.toMatchSnapshot() }

However this changes the snapshots dramatically, and it looks like we lose some critical information stored in the snapshots. So it might not be a good idea to change this after all.

Related thread: jestjs/jest#3601

paulmolluzzo · 2018-02-28T14:08:09Z

packages/data-point/lib/debug-utils/stack.test.js

+      }
+    }
+  }
+})


I definitely see the value of defining these entities here, but it's not the same pattern as the other entity fixtures we have (in the test directory).

I'm not asking this to be changed or moved, just commenting in case there's interest in discussing the inclusion of this in this file.

I like defining them in the same file so it's easier to understand the tests, and since these entities are testing very specific cases, I doubt they would ever be used in other files.

If we do use the same entities in multiple tests (which is encouraged by defining them in the test directory), it could also make it harder to make changes to the entities if we ever need to, since any change would affect multiple tests.

Lastly, if every instance of datapoint that we create for the tests contains all if the entities defined in the test directory, that might slow down the tests (since we probably create a lot of instances).

paulmolluzzo · 2018-02-28T14:10:06Z

packages/data-point/lib/debug-utils/stack.test.js

+dataPoint.addEntities(schemaA10)
+
+describe('transform entity stack traces', () => {
+  test('transform:1', () => {


The descriptions could use a little improvement (even though I expect they'll all say something similar). Just thinking that if this is run with --verbose there's no additional information from the descriptions.

I went through and added better descriptions; please take a look when you have a chance

paulmolluzzo · 2018-02-28T14:17:10Z

packages/data-point/lib/entity-types/entity-entry/resolve.js

-  racc = utils.set(acc, 'value', _.defaultTo(acc.value, {}))
-  return resolveReducer(racc, contextTransform)
+  const racc = utils.set(acc, 'value', _.defaultTo(acc.value, {}))
+  return resolveReducer(racc, contextTransform, [['value']])


These nested arrays are in a few places. I'm still not exactly sure why, tbh. What's the format that is produced and why is it being done that way? (Sorry if I'm overlooking something obvious.)

The format can be seen by looking at the _stack strings in the snapshots.

The values in the nested arrays will be printed with brackets and no preceding whitespace, so that we get things like 'entry:example[value]' (and the strings are created by the stringifyReducerStack function)

paulmolluzzo · 2018-02-28T14:19:22Z

packages/data-point/lib/reducer-types/reducer-path/resolve.js

@@ -1,3 +1,5 @@
+const Promise = require('bluebird')


Is this a required change?

added this because I was getting an error when trying to use Promise.tap in the main reducer resolve function - I'm no longer using tap, but left this in

paulmolluzzo · 2018-02-28T14:21:33Z

packages/data-point/lib/reducer-types/resolve.js

+    stack = castArray(key)
+  }
+
+  if (reducer.type === 'ReducerFunction') {


Might this be better as a switch? I don't know what other types we might have a case for, but at a certain point if..else if..else gets cumbersome to read.

We probably won't need more conditions here, since I don't think we'll be adding many more reducer types. Honestly I just really dislike switch syntax...

Honestly I just really dislike switch syntax...

Ah ha!!!! 😄

But for real, if we're not going to extend it then a switch isn't really necessary I guess.

Happy to change it though if there's a consensus...

It might be premature to change it now if we only have to account for these two scenarios.

paulmolluzzo · 2018-02-28T14:22:04Z

packages/data-point/lib/reducer-types/resolve.test.js

+      .catch(e => e)
+      .then(result => {
+        expect(result).toBeInstanceOf(Error)
+        expect(result).toMatchSnapshot()


Maybe we want to use toThrowErrorMatchingSnapshot() here?

Just for record keeping, we agreed not to do this because it's using a promise instead of a synchronous function.

raingerber · 2018-03-06T14:15:15Z

packages/data-point/lib/entity-types/entity-control/resolve.js

 */
-function getMatchingCaseStatement (caseStatements, acc, resolveReducer) {
+function getMatchingCaseIndex (caseStatements, accumulator, resolveReducer) {


returning the index instead of the statement, so that we can add the index to the reducer stack

codecov · 2018-03-10T13:26:44Z

Codecov Report

Merging #230 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #230   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         129    130    +1     
  Lines        1700   1737   +37     
  Branches      149    160   +11     
=====================================
+ Hits         1700   1737   +37

Impacted Files	Coverage Δ
packages/data-point/lib/reducer-types/factory.js	`100% <ø> (ø)`	⬆️
...ucer-types/reducer-helpers/reducer-find/resolve.js	`100% <ø> (ø)`	⬆️
...ata-point/lib/entity-types/entity-entry/resolve.js	`100% <100%> (ø)`	⬆️
...a-point/lib/entity-types/entity-request/resolve.js	`100% <100%> (ø)`	⬆️
...ucer-types/reducer-helpers/reducer-pick/resolve.js	`100% <100%> (ø)`	⬆️
...oint/lib/reducer-types/reducer-function/factory.js	`100% <100%> (ø)`	⬆️
...data-point/lib/entity-types/entity-hash/resolve.js	`100% <100%> (ø)`	⬆️
...ata-point/lib/entity-types/entity-model/resolve.js	`100% <100%> (ø)`	⬆️
...-types/reducer-helpers/reducer-parallel/resolve.js	`100% <100%> (ø)`	⬆️
packages/data-point/lib/reducer-types/resolve.js	`100% <100%> (ø)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d212e83...692ce80. Read the comment docs.

… is set

raingerber and others added 2 commits February 23, 2018 16:10

test(data-point/request): Document and add tests for EntityRequest#va…

db70d33

…lue method (ViacomInc#222)

Merge branch 'master' into reducer-stack-second-attempt

1141720

raingerber self-assigned this Feb 23, 2018

test: Fix tests for error._stack properties

6b06463

refactor(data-point): Improvements to reducer stack PR

20181e9

raingerber commented Feb 23, 2018

View reviewed changes

docs: Reword some comments and tests

fe94161

raingerber commented Feb 23, 2018

View reviewed changes

Matthew Armstrong added 6 commits February 23, 2018 18:12

docs(base-entity): Fix jsdoc

aa525b5

refactor(entity-schema): Wrap validateContext in a reducer

739935b

This allows for easier reducer stack traces

docs(reducer-types): Improve a comment

d7acd79

Merge branch 'master' into reducer-stack-second-attempt

1087885

style: Use explicit return in arrow function

e8fce24

Merge branch 'master' into reducer-stack-second-attempt

4bda533

raingerber closed this Feb 26, 2018

raingerber reopened this Feb 27, 2018

paulmolluzzo requested changes Feb 28, 2018

View reviewed changes

Matthew Armstrong added 4 commits February 28, 2018 10:48

Merge branch 'master' into reducer-stack-second-attempt

89320fb

feat: Rename _value variable to _input

d614af4

docs: Update a comment describing ReducerFunction behavior

178678d

test(stack.test.js): Improve test descriptions

4adaea5

raingerber commented Mar 6, 2018

View reviewed changes

Merge branch 'master' into reducer-stack-second-attempt

79559b8

fix(reducer-types): Call onResolveMalfunction even if a default value…

692ce80

… is set

raingerber closed this Mar 11, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(data-point): Add reducer stack when reducers throw errors #230

feat(data-point): Add reducer stack when reducers throw errors #230

raingerber commented Feb 23, 2018 •

edited

Loading

coveralls commented Feb 23, 2018 •

edited

Loading

raingerber Feb 23, 2018

raingerber Feb 23, 2018 •

edited

Loading

raingerber Feb 23, 2018

paulmolluzzo Feb 28, 2018

raingerber Mar 2, 2018

raingerber Feb 23, 2018 •

edited

Loading

paulmolluzzo Feb 28, 2018

paulmolluzzo left a comment

paulmolluzzo Feb 28, 2018

raingerber Feb 28, 2018

paulmolluzzo Feb 28, 2018

raingerber Feb 28, 2018

paulmolluzzo Feb 28, 2018

paulmolluzzo Feb 28, 2018

raingerber Feb 28, 2018

paulmolluzzo Feb 28, 2018

raingerber Mar 2, 2018

paulmolluzzo Feb 28, 2018

raingerber Feb 28, 2018

raingerber Mar 2, 2018 •

edited

Loading

paulmolluzzo Feb 28, 2018

raingerber Feb 28, 2018

paulmolluzzo Feb 28, 2018

raingerber Feb 28, 2018

paulmolluzzo Feb 28, 2018

raingerber Feb 28, 2018

paulmolluzzo Mar 1, 2018

paulmolluzzo Feb 28, 2018

raingerber Mar 2, 2018 •

edited

Loading

raingerber Mar 6, 2018

codecov bot commented Mar 10, 2018 •

edited

Loading


		When a reducer throws an error, DataPoint adds two properties to the error object:

		`_value:*` - the input value to the reducer that failed

feat(data-point): Add reducer stack when reducers throw errors #230

feat(data-point): Add reducer stack when reducers throw errors #230

Conversation

raingerber commented Feb 23, 2018 • edited Loading

coveralls commented Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

raingerber Feb 23, 2018 • 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

raingerber Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmolluzzo left a 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

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

raingerber Mar 2, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raingerber Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 10, 2018 • edited Loading

Codecov Report

raingerber commented Feb 23, 2018 •

edited

Loading

coveralls commented Feb 23, 2018 •

edited

Loading

raingerber Feb 23, 2018 •

edited

Loading

raingerber Feb 23, 2018 •

edited

Loading

raingerber Mar 2, 2018 •

edited

Loading

raingerber Mar 2, 2018 •

edited

Loading

codecov bot commented Mar 10, 2018 •

edited

Loading