Repository: ViacomInc/data-point
Pull request #409 (2019-08-29 21:16:12)

Add reducer tracing to errors
<!--
Thanks for your interest in the project. We appreciate bugs filed and PRs submitted!

Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Also, please make sure you're familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!

BREAKING CHANGES:

If your PR includes a breaking change, please submit it with a codemod under
data-point-codemods that will help users upgrade their codebase.

Breaking changes without a codemod will not be accepted unless a codemod is not
viable or does not apply to the specific situation.
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->
**What**:

Exposes a reducerStackTrace when an error occurs on the execution of any reducer.

<!-- Why are these changes necessary? -->
**Why**:

To help us debug errors and follow the trace of reducers executed. 

<!-- Have you done all of these things?  -->
**Checklist**:
<!-- add "N/A" to the end of each line that's irrelevant to your changes -->
<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->
- [ ] Has Breaking changes
- [ ] Documentation
- [X] Tests
- [X] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [ ] Added username to **all-contributors** list

<!-- feel free to add additional comments -->

Notice reducerStackTrace error property being logged: 

<img width="1047" alt="image" src="https://user-images.githubusercontent.com/235111/63977124-f36ad400-ca80-11e9-96ba-599b544bfde3.png">



codecov[bot]: # [Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/409?src=pr&el=h1) Report
> Merging [#409](https://codecov.io/gh/ViacomInc/data-point/pull/409?src=pr&el=desc) into [canary](https://codecov.io/gh/ViacomInc/data-point/commit/b4b9a63d5bb4415900f8ab71ddc5ab0c97743a91?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/ViacomInc/data-point/pull/409/graphs/tree.svg?width=650&token=LYxItnrx4i&height=150&src=pr)](https://codecov.io/gh/ViacomInc/data-point/pull/409?src=pr&el=tree)

```diff
@@          Coverage Diff          @@
##           canary   #409   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          24     24           
  Lines         388    394    +6     
  Branches       57     58    +1     
=====================================
+ Hits          388    394    +6
```


| [Impacted Files](https://codecov.io/gh/ViacomInc/data-point/pull/409?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [packages/data-point/src/Accumulator.js](https://codecov.io/gh/ViacomInc/data-point/pull/409/diff?src=pr&el=tree#diff-cGFja2FnZXMvZGF0YS1wb2ludC9zcmMvQWNjdW11bGF0b3IuanM=) | `100% <100%> (ø)` | :arrow_up: |
| [packages/data-point/src/resolve.js](https://codecov.io/gh/ViacomInc/data-point/pull/409/diff?src=pr&el=tree#diff-cGFja2FnZXMvZGF0YS1wb2ludC9zcmMvcmVzb2x2ZS5qcw==) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/409?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/409?src=pr&el=footer). Last update [b4b9a63...bb76637](https://codecov.io/gh/ViacomInc/data-point/pull/409?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/packages/data-point/src/Accumulator.js b/packages/data-point/src/Accumulator.js
index 56c0c3d..39a761b 100644
--- a/packages/data-point/src/Accumulator.js
+++ b/packages/data-point/src/Accumulator.js
@@ -30,9 +30,9 @@ class Accumulator {
     this.tracer = options.tracer;
 
     /**
-     * @type {Reducer} reference to the current reducer
+     * @type {string[]} stores the list of reducers executed per branch.
      */
-    this.reducer = options.reducer;
+    this.__reducerStackTrace = [];
 
     /**
      * @type {Number} current reducer process id, this number changes on the
@@ -59,6 +59,23 @@ class Accumulator {
     return copy;
   }
 
+  set reducer(reducer) {
+    this.__reducer = reducer;
+
+    // we create a new array to create a new reducer history branch.
+    this.__reducerStackTrace = this.__reducerStackTrace.concat(
+      this.__reducer.id
+    );
+  }
+
+  get reducer() {
+    return this.__reducer;
+  }
+
+  get reducerStackTrace() {
+    return this.__reducerStackTrace.join(" > ");
+  }
+
   resolve(reducers) {
     return this.__resolve(this, reducers);
   }
diff --git a/packages/data-point/src/resolve.js b/packages/data-point/src/resolve.js
index 2142a83..88bb82e 100644
--- a/packages/data-point/src/resolve.js
+++ b/packages/data-point/src/resolve.js
@@ -22,7 +22,15 @@ async function resolve(accumulator, reducer) {
     // NOTE: recursive call by passing resolve method
     result = await reducer.resolveReducer(acc, resolve);
   } catch (error) {
+    // because the error bubbles up, we only want to set `error.reducerStackTrace`
+    // once, otherwise we end up with only the first item in the stack.
+    if (!error.reducerStackTrace) {
+      error.reducerStackTrace = acc.reducerStackTrace;
+    }
+
     traceSpan.logError(span, error);
+
+    // rethrow the error up
     throw error;
   } finally {
     traceSpan.finish(span);


Test patch:
diff --git a/packages/data-point/src/Accumulator.test.js b/packages/data-point/src/Accumulator.test.js
index cb99ebc..038a6d8 100644
--- a/packages/data-point/src/Accumulator.test.js
+++ b/packages/data-point/src/Accumulator.test.js
@@ -5,15 +5,16 @@ describe("Accumulator", () => {
     it("should accept empty options object", () => {
       expect(new Accumulator()).toMatchInlineSnapshot(`
         Accumulator {
+          "__reducerStackTrace": Array [],
           "cache": Object {},
           "locals": undefined,
           "pid": undefined,
-          "reducer": undefined,
           "tracer": undefined,
           "value": undefined,
         }
       `);
     });
+
     it("should assign options object", () => {
       const resolve = () => true;
       const acc = new Accumulator({
@@ -23,12 +24,13 @@ describe("Accumulator", () => {
         tracer: "tracer",
         resolve
       });
+
       expect(acc).toMatchInlineSnapshot(`
         Accumulator {
+          "__reducerStackTrace": Array [],
           "cache": "cache",
           "locals": "locals",
           "pid": undefined,
-          "reducer": undefined,
           "tracer": "tracer",
           "value": "value",
         }
@@ -60,6 +62,43 @@ describe("Accumulator", () => {
       expect(Object.getPrototypeOf(acc2)).toEqual(acc1);
     });
   });
+
+  describe("reducer", () => {
+    const reducer = {
+      id: "myReducerId"
+    };
+
+    describe("set reducer", () => {
+      it("should append a new reducer to the stack of reducers", () => {
+        const acc = new Accumulator({
+          value: "a"
+        });
+
+        // eslint-disable-next-line no-underscore-dangle
+        const originalReducerStackTrace = acc.__reducerStackTrace;
+
+        acc.reducer = reducer;
+
+        // should not mutate the original stackTrace
+        expect(originalReducerStackTrace).toEqual([]);
+        // eslint-disable-next-line no-underscore-dangle
+        expect(acc.__reducerStackTrace).toEqual(["myReducerId"]);
+      });
+    });
+
+    describe("get reducer", () => {
+      it("should return stored internal reducer", () => {
+        const acc = new Accumulator({
+          value: "a"
+        });
+
+        acc.reducer = reducer;
+        // eslint-disable-next-line no-underscore-dangle
+        expect(acc.__reducer).toEqual(reducer);
+      });
+    });
+  });
+
   describe("resolve", () => {
     it("should call internal resolve method", () => {
       const mockResolve = jest.fn(() => "resolved");
diff --git a/packages/data-point/src/resolve.test.js b/packages/data-point/src/resolve.test.js
index 1564928..5869892 100644
--- a/packages/data-point/src/resolve.test.js
+++ b/packages/data-point/src/resolve.test.js
@@ -35,6 +35,41 @@ describe("resolve", () => {
     });
   });
 
+  describe("reducerStackTrace", () => {
+    it("should augment error with reducer stacktrace", async () => {
+      const acc = new Accumulator();
+
+      const testError = new Error("resolveReducer failed");
+
+      const reducerRejected = {
+        id: "badReducer",
+        resolveReducer: jest.fn().mockRejectedValue(testError)
+      };
+
+      const result = await resolve(acc, reducerRejected).catch(error => error);
+
+      expect(result).toBeInstanceOf(Error);
+      expect(result.reducerStackTrace).toEqual("badReducer");
+    });
+
+    it("should not augment error with reducer stacktrace if already set", async () => {
+      const acc = new Accumulator();
+
+      const testError = new Error("resolveReducer failed");
+      testError.reducerStackTrace = "initialReducerStackTrace";
+
+      const reducerRejected = {
+        id: "badReducer",
+        resolveReducer: jest.fn().mockRejectedValue(testError)
+      };
+
+      const result = await resolve(acc, reducerRejected).catch(error => error);
+
+      expect(result).toBeInstanceOf(Error);
+      expect(result.reducerStackTrace).toEqual("initialReducerStackTrace");
+    });
+  });
+
   describe("tracing", () => {
     it("should call traceSpan.create with accumulator", async () => {
       const acc = new Accumulator();
@@ -49,25 +84,16 @@ describe("resolve", () => {
     it("should call traceSpan.logError when resolveReducer throws error", async () => {
       const acc = new Accumulator();
 
+      const testError = new Error("resolveReducer failed");
       const reducerRejected = {
-        resolveReducer: jest
-          .fn()
-          .mockRejectedValue(new Error("resolveReducer failed"))
+        resolveReducer: jest.fn().mockRejectedValue(testError)
       };
 
       await resolve(acc, reducerRejected).catch(error => error);
 
       expect(traceSpan.logError).toBeCalled();
 
-      // first argument would is an undefined span
-      expect(traceSpan.logError.mock.calls).toMatchInlineSnapshot(`
-                Array [
-                  Array [
-                    undefined,
-                    [Error: resolveReducer failed],
-                  ],
-                ]
-            `);
+      expect(traceSpan.logError.mock.calls[0]).toEqual([undefined, testError]);
     });
 
     it("should finally call traceSpan.finish()", async () => {