fix: capture error stacks across async calls #1088

thebrianchen · 2020-05-21T18:24:30Z

Tested for get/set operations on references, queries, batches, and bulkCommit.

schmidt-sebastian

You showed me a test case for this earlier - would it be possible to add a single System Test for one of the entry points?

I also wonder if Errror().stack generates some overhead since the documentation says that "stack" is lazily generated. If you have time, can you benchmark a single call?

schmidt-sebastian · 2020-05-21T19:16:06Z

dev/src/bulk-writer.ts

-    return this.writeBatch.bulkCommit();
+
+    // Capture the error stack to preserve stack tracing across async calls.
+    const stack = Error().stack;


Nit: Move bang here.

schmidt-sebastian · 2020-05-21T19:16:17Z

dev/src/index.ts

@@ -908,9 +908,15 @@ export class Firestore {
      documentRefsOrReadOptions
    );
    const tag = requestTag();
+
+    // Capture the error stack to preserve stack tracing across async calls.
+    const stack = Error().stack;


Nit: Move bang here.

schmidt-sebastian · 2020-05-21T19:16:31Z

dev/src/pool.ts

@@ -17,7 +17,7 @@
 import * as assert from 'assert';

 import {logger} from './logger';
-import {Deferred} from './util';
+import {Deferred, wrapError} from './util';


Unused import?

schmidt-sebastian · 2020-05-21T19:17:04Z

dev/src/reference.ts

@@ -1808,12 +1808,15 @@ export class Query<T = DocumentData> {
  _get(transactionId?: Uint8Array): Promise<QuerySnapshot<T>> {
    const docs: Array<QueryDocumentSnapshot<T>> = [];

+    // Capture the error stack to preserve stack tracing across async calls.
+    const stack = Error().stack;


Nit: Move bang here.

schmidt-sebastian · 2020-05-21T19:17:23Z

dev/src/write-batch.ts

@@ -532,7 +532,13 @@ export class WriteBatch {
   * });
   */
  commit(): Promise<WriteResult[]> {
-    return this.commit_();
+


Nit: Remove empty line.

schmidt-sebastian · 2020-05-21T19:17:28Z

dev/src/write-batch.ts

-    return this.commit_();
+
+    // Capture the error stack to preserve stack tracing across async calls.
+    const stack = Error().stack;


Nit: Move bang here.

thebrianchen

Did some benchmarking. Across 2500 calls to ref.get() the average RTT is 63.4ms for stack tracing vs. 60.1ms for no stack tracing. The spread was pretty wide though, with times in the +/- 10ms range. Calls to ref.set() were similar.

thebrianchen · 2020-05-21T22:06:26Z

dev/src/bulk-writer.ts

-    return this.writeBatch.bulkCommit();
+
+    // Capture the error stack to preserve stack tracing across async calls.
+    const stack = Error().stack;


thebrianchen · 2020-05-21T22:06:48Z

dev/src/index.ts

@@ -908,9 +908,15 @@ export class Firestore {
      documentRefsOrReadOptions
    );
    const tag = requestTag();
+
+    // Capture the error stack to preserve stack tracing across async calls.
+    const stack = Error().stack;


thebrianchen · 2020-05-21T22:07:09Z

dev/src/pool.ts

@@ -17,7 +17,7 @@
 import * as assert from 'assert';

 import {logger} from './logger';
-import {Deferred} from './util';
+import {Deferred, wrapError} from './util';


thebrianchen · 2020-05-21T22:07:27Z

dev/src/reference.ts

@@ -1808,12 +1808,15 @@ export class Query<T = DocumentData> {
  _get(transactionId?: Uint8Array): Promise<QuerySnapshot<T>> {
    const docs: Array<QueryDocumentSnapshot<T>> = [];

+    // Capture the error stack to preserve stack tracing across async calls.
+    const stack = Error().stack;


thebrianchen · 2020-05-21T22:07:48Z

dev/src/write-batch.ts

@@ -532,7 +532,13 @@ export class WriteBatch {
   * });
   */
  commit(): Promise<WriteResult[]> {
-    return this.commit_();
+


thebrianchen · 2020-05-21T22:07:52Z

dev/src/write-batch.ts

-    return this.commit_();
+
+    // Capture the error stack to preserve stack tracing across async calls.
+    const stack = Error().stack;


schmidt-sebastian · 2020-05-22T00:41:27Z

dev/system-test/firestore.ts

+    const ref = randomCol.doc('__doc__');
+    const batch = firestore.batch();
+    batch.set(ref, {foo: 'a'});
+    return batch.commit().then(() => Promise.reject('commit() should have failed')


I am glad this is causing a lint failure :) Doesn't look pretty like this. The PR is good to go after yarn gts fix.

fixed. Also I changed wrapError() to use GoogleError in order to preserve error codes when re-throwing.

…re into bc/error-stack

codecov · 2020-05-22T01:17:49Z

Codecov Report

Merging #1088 into master will decrease coverage by 0.14%.
The diff coverage is 93.47%.

@@            Coverage Diff             @@
##           master    #1088      +/-   ##
==========================================
- Coverage   97.62%   97.48%   -0.15%     
==========================================
  Files          27       27              
  Lines       16924    16960      +36     
  Branches     1347     1267      -80     
==========================================
+ Hits        16522    16533      +11     
- Misses        399      423      +24     
- Partials        3        4       +1

Impacted Files	Coverage Δ
dev/src/util.ts	`98.17% <81.25%> (-1.83%)`	⬇️
dev/src/bulk-writer.ts	`98.49% <100.00%> (-0.30%)`	⬇️
dev/src/index.ts	`98.60% <100.00%> (-0.07%)`	⬇️
dev/src/reference.ts	`99.80% <100.00%> (-0.08%)`	⬇️
dev/src/write-batch.ts	`99.89% <100.00%> (-0.11%)`	⬇️
dev/src/order.ts	`98.09% <0.00%> (-1.15%)`	⬇️
dev/src/pool.ts	`97.75% <0.00%> (-0.90%)`	⬇️
dev/src/transaction.ts	`96.47% <0.00%> (-0.51%)`	⬇️
dev/src/watch.ts	`98.77% <0.00%> (-0.37%)`	⬇️
... and 6 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 ad6b923...c471b2d. Read the comment docs.

schmidt-sebastian · 2020-05-22T01:38:07Z

dev/src/util.ts

+export function wrapError(err: GoogleError, stack: string): Error {
+  const wrappedError = new GoogleError(err.message);
+  wrappedError.code = err.code;
+  wrappedError.stack += '\nCaused by: ' + stack;


Can we modify the stack in 'err' directly?

good point. done.

schmidt-sebastian

LGTM

fix: capture error stacks across async calls

294ac2e

thebrianchen self-assigned this May 21, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 21, 2020

thebrianchen requested a review from schmidt-sebastian May 21, 2020 18:25

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen May 21, 2020

schmidt-sebastian reviewed May 21, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian May 21, 2020

add comments + test

541449a

thebrianchen commented May 21, 2020

View reviewed changes

thebrianchen force-pushed the bc/error-stack branch from d99b124 to 541449a Compare May 21, 2020 23:10

Merge branch 'master' into bc/error-stack

aaf2c36

thebrianchen requested a review from schmidt-sebastian May 21, 2020 23:11

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen May 21, 2020

schmidt-sebastian approved these changes May 22, 2020

View reviewed changes

schmidt-sebastian reviewed May 22, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian May 22, 2020

Brian Chen added 2 commits May 21, 2020 17:48

lint

9da29bf

Merge branch 'bc/error-stack' of github.com:googleapis/nodejs-firesto…

2b99046

…re into bc/error-stack

thebrianchen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 22, 2020

change to GoogleError to preserve error code

3a4d421

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 22, 2020

schmidt-sebastian reviewed May 22, 2020

View reviewed changes

modify error stack in place rather than create new error

5523007

schmidt-sebastian approved these changes May 26, 2020

View reviewed changes

thebrianchen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 26, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 26, 2020

throw strings with TODO

d53aefb

schmidt-sebastian approved these changes May 26, 2020

View reviewed changes

lint

c471b2d

thebrianchen merged commit 7acdd7e into master May 27, 2020

thebrianchen deleted the bc/error-stack branch May 27, 2020 00:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: capture error stacks across async calls #1088

fix: capture error stacks across async calls #1088

thebrianchen commented May 21, 2020

schmidt-sebastian left a comment

schmidt-sebastian May 21, 2020

thebrianchen May 21, 2020

schmidt-sebastian May 21, 2020

thebrianchen May 21, 2020

schmidt-sebastian May 21, 2020

thebrianchen May 21, 2020

schmidt-sebastian May 21, 2020

thebrianchen May 21, 2020

schmidt-sebastian May 21, 2020

thebrianchen May 21, 2020

schmidt-sebastian May 21, 2020

thebrianchen May 21, 2020

thebrianchen left a comment

thebrianchen May 21, 2020

thebrianchen May 21, 2020

thebrianchen May 21, 2020

thebrianchen May 21, 2020

thebrianchen May 21, 2020

thebrianchen May 21, 2020

schmidt-sebastian May 22, 2020

thebrianchen May 22, 2020

codecov bot commented May 22, 2020 •

edited

schmidt-sebastian May 22, 2020

thebrianchen May 26, 2020

schmidt-sebastian left a comment

fix: capture error stacks across async calls #1088

fix: capture error stacks across async calls #1088

Conversation

thebrianchen commented May 21, 2020

schmidt-sebastian 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

Choose a reason for hiding this comment

thebrianchen 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

codecov bot commented May 22, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

codecov bot commented May 22, 2020 •

edited