feat(observability): fix bugs found from product review + negative cases by odeke-em · Pull Request #2158 · googleapis/nodejs-spanner

odeke-em · 2024-10-12T09:25:49Z

This change adds recording of retry span annotations, catching cases in which exceptions where thrown but spans were not ended while testing out and visually confirming the results.

src/transaction.ts

alkatrivedi · 2024-10-17T05:18:31Z

src/database.ts

-          if (err) {
-            setSpanError(span, err!);
-          }
+        await runner.run().then(release, async err => {


we are already doing .then() on runner.run(), why do we then need to await here?

@odeke-em it will work even if we don't await on runner.run(). please check and remove the await

src/database.ts

surbhigarg92 · 2024-10-14T05:06:54Z

src/transaction.ts

              (err as grpc.ServiceError).code === grpc.status.ABORTED
            )
          ) {
+            span.addEvent('Transaction Aborted, retrying begin', {


This code block is for the non Aborted errors. Change Transaction threw error, retrying with explicit begin

surbhigarg92 · 2024-10-17T12:58:03Z

src/database.ts

            'session.id': session?.id,
          });
-          this.runTransaction(options, runFn!);
+          await this.runTransaction(options, runFn!);


Why have we added await on this.runTransaction. This does not return a promise nor its a async method.

surbhigarg92 · 2024-10-17T12:58:22Z

src/database.ts

        }

        if (err) {
+          await runFn!(err as grpc.ServiceError);


same here ?

surbhigarg92 · 2024-10-17T12:59:36Z

src/database.ts

            });
            release();
-            this.runTransaction(options, runFn!);
+            await this.runTransaction(options, runFn!);


again why async here?

surbhigarg92 · 2024-10-17T13:02:06Z

src/database.ts

+
+            try {
+              const result = await runner.run();
+              span.end();


span.end() should be done in finally block.

Not really, on an exception this code retries in the while loop above so that's not correct to end it in the finally block. It should be ended only on success.

src/transaction.ts

surbhigarg92 · 2024-10-17T13:39:44Z

src/transaction.ts

            )
          ) {
-            this.begin();
+            span.addEvent('Stream broken. Safe to retry', {


remove these two spans for stream broken. We are not retrying broken stream here , that is happening in partial-result-stream class. https://github.com/googleapis/nodejs-spanner/blob/main/src/partial-result-stream.ts#L558-L586
Maybe lets skip these two spans for now and move on.

src/transaction.ts

surbhigarg92 · 2024-10-17T13:42:17Z

src/transaction.ts

            )
          ) {
-            this.begin();
+            span.addEvent('Stream broken. Safe to retry', {


same here. remove these events

surbhigarg92 · 2024-10-17T13:43:06Z

src/transaction.ts

-            callback(err, resp);
-          });
-        }, callback);
+        try {


no change is required here. As we are doing ".then" on begin call.

Extracted out of PR googleapis#2158, this change traces Database.runTransactionAsync. However, testing isn't effective because of bugs such as googleapis#2166.

Extracted out of PR googleapis#2158, this change traces Database.runTransactionAsync. Updates googleapis#2079

Extracted out of PR #2158, this change traces Database.runTransactionAsync. Updates #207

…LLED

surbhigarg92 · 2024-10-24T06:46:23Z

src/database.ts

+          .catch(e => {
+            setSpanErrorAndException(span, e as Error);
+            throw e;
+          });


Suggested change

.catch(e => {

setSpanErrorAndException(span, e as Error);

throw e;

});

.catch(e => {

setSpanErrorAndException(span, e as Error);

span.end();

throw e;

});

…back response

Extracted out of PR googleapis#2158, this change traces Database.runTransactionAsync. Updates googleapis#2079

odeke-em requested review from a team as code owners October 12, 2024 09:25

product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/nodejs-spanner API. labels Oct 12, 2024

odeke-em force-pushed the trace-fix-bugs-from-product-review+negative-cases branch from 62bf905 to 1b7d8b6 Compare October 12, 2024 09:26

product-auto-label bot added size: l Pull request size is large. size: xl Pull request size is extra large. and removed size: m Pull request size is medium. size: l Pull request size is large. labels Oct 12, 2024

alkatrivedi reviewed Oct 16, 2024

View reviewed changes

src/transaction.ts Outdated Show resolved Hide resolved

alkatrivedi reviewed Oct 17, 2024

View reviewed changes

src/database.ts Outdated Show resolved Hide resolved

surbhigarg92 reviewed Oct 17, 2024

View reviewed changes

odeke-em force-pushed the trace-fix-bugs-from-product-review+negative-cases branch from 91c5281 to 508243f Compare October 18, 2024 06:06

alkatrivedi added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Oct 18, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 18, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 18, 2024

alkatrivedi added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Oct 18, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 18, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 18, 2024

odeke-em force-pushed the trace-fix-bugs-from-product-review+negative-cases branch from 7bdc173 to a3589e5 Compare October 20, 2024 03:28

odeke-em mentioned this pull request Oct 21, 2024

feat: (observability): trace Database.runTransactionAsync #2167

Merged

odeke-em added a commit to odeke-em/nodejs-spanner that referenced this pull request Oct 21, 2024

feat: (observability): trace Database.runTransactionAsync

4ecc6b4

Extracted out of PR googleapis#2158, this change traces Database.runTransactionAsync. Updates googleapis#2079

odeke-em force-pushed the trace-fix-bugs-from-product-review+negative-cases branch from e0e31f5 to 13bc9f5 Compare October 21, 2024 10:52

gcf-merge-on-green bot pushed a commit that referenced this pull request Oct 21, 2024

feat: (observability): trace Database.runTransactionAsync (#2167)

d0fe178

Extracted out of PR #2158, this change traces Database.runTransactionAsync. Updates #207

odeke-em force-pushed the trace-fix-bugs-from-product-review+negative-cases branch from 3fb5164 to 03a6298 Compare October 21, 2024 13:15

odeke-em added 6 commits October 23, 2024 22:03

De-flake EndToEnd Database.runStream test with by ignoring grpc.CANCE…

13cc509

…LLED

test: remove async vestige

d86986b

Attempt to isolate and de-flake

76bd38d

More test failure debugs

bea444c

hotfix to allow tests to run without blocking testers

7128de7

Address feedback from code review

d10a888

alkatrivedi added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Oct 24, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 24, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 24, 2024

EndToEnd test setup reduced to avoid flakes

9b0a458

odeke-em force-pushed the trace-fix-bugs-from-product-review+negative-cases branch from ab6ae5d to 9b0a458 Compare October 24, 2024 06:38

Revert Database.runTransaction runner ordering

0458064

surbhigarg92 reviewed Oct 24, 2024

View reviewed changes

surbhigarg92 approved these changes Oct 24, 2024

View reviewed changes

surbhigarg92 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 24, 2024

yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 24, 2024

surbhigarg92 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 24, 2024

yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 24, 2024

Uncommitted change for Database.getSnapshot: end span before sending …

f0b8745

…back response

odeke-em force-pushed the trace-fix-bugs-from-product-review+negative-cases branch from 5bf4980 to f0b8745 Compare October 24, 2024 07:50

surbhigarg92 added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Oct 24, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 24, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 24, 2024

surbhigarg92 merged commit cbc86fa into googleapis:main Oct 24, 2024

release-please bot mentioned this pull request Oct 24, 2024

chore(main): release 7.15.0 #2118

Merged

odeke-em deleted the trace-fix-bugs-from-product-review+negative-cases branch October 24, 2024 18:42

surbhigarg92 pushed a commit to surbhigarg92/nodejs-spanner that referenced this pull request Oct 25, 2024

feat: (observability): trace Database.runTransactionAsync

f6fa667

Extracted out of PR googleapis#2158, this change traces Database.runTransactionAsync. Updates googleapis#2079

Conversation

odeke-em commented Oct 12, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants