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.

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

-          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

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

              (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

            '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

        }

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


same here ?

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

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


again why async here?

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

+
+            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.

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

            )
          ) {
-            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.

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

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


same here. remove these events

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

-            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

odeke-em · 2024-10-23T09:41:50Z

@alkatrivedi kindly help me run the bots.

odeke-em · 2024-10-23T10:00:45Z

Kind ping @alkatrivedi to run the bots.

odeke-em · 2024-10-23T10:08:14Z

Kind ping @harshachinta @alkatrivedi @surbhigarg92 to run the bots as I need rapid debugging but the time in-between is too long.

surbhigarg92 · 2024-10-23T12:08:40Z

        setSpanErrorAndException(span, e as Error);
        this.emit('error', e);
+      } finally {
+        if (span.isRecording()) {


Please remove span. isRecording

surbhigarg92 · 2024-10-23T12:13:56Z

-                span.end();
-                callback!(err, snapshot);
-              });
+              this.getSnapshot(options, callback!);


move span.end() above the callback. We will not see the traces hierarchy in this case which is ok for sessionnotfounderror.

surbhigarg92 · 2024-10-23T12:14:08Z

-                callback!(err, snapshot);
-              });
+              this.getSnapshot(options, callback!);
+              // Explicitly requested in code review that this span.end() be


Please remove these comments from code

surbhigarg92 · 2024-10-23T12:19:23Z

              dataStream = this.runStream(query, options);
              dataStream.pipe(proxyStream);
+              // Explicitly invoking span.end() here,
+              // instead of inside dataStream.on('error').
+              span.end();


Move span.end() to before we create new datastream

surbhigarg92 · 2024-10-23T12:47:55Z

-            }
            setImmediate(runFn!, err);
            release();
+            span.end();


remove span.end() once we add it in release method

surbhigarg92 · 2024-10-23T12:48:38Z

                  options
                );
                dataStream.pipe(proxyStream);
+                span.end();


move this span.end() before creating new stream

surbhigarg92 · 2024-10-23T12:49:49Z

+          // Retry this method.
+          this.writeAtLeastOnce(mutations, options, (err, resp) => {
+            if (err) {
+              setSpanError(span, err);
+            }
+            span.end();
+            cb!(err, resp);
+          });


We can remove this change and add span.end() before retry

Done, thanks!

surbhigarg92 · 2024-10-23T13:01:18Z

+            // TODO: re-examine if this.begin() should still exist and if
+            // an await is needed: with await the generated begin span
+            // will look wonky and out of order. Please ses


Please add a short comment, bug has details

Done, thanks!

surbhigarg92 · 2024-10-23T13:02:01Z

+            // TODO: Examine the consequence of not awaiting this method,
+            // as the span might appear out of order.
+            // Please see https://github.com/googleapis/nodejs-spanner/issues/2170


Same as above

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.

…LLED

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

+          .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

odeke-em requested review from a team 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

Comment thread src/transaction.ts Outdated

alkatrivedi reviewed Oct 17, 2024

View reviewed changes

Comment thread src/database.ts Outdated

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

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 23, 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 23, 2024

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

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

surbhigarg92 reviewed Oct 23, 2024

View reviewed changes

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

feat(observability): fix bugs found from product review + negative cases

828b962

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.

Address merge from main

f3df288

Ensure Database.getSnapshot correct span ending + tests

ed4a345

Proper placement of Transaction span.end()

1d17cbb

Adjust tests to ensure transaction.end() where necessary

b8d91b8

Address some review feedback

0205732

Handle moving 2nd getSnapshot() retry to itself

e7e84e3

Remove dataStream.once span nesting

d1ec79b

End spans even on dataStream retry

abfc373

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

EndToEnd test setup reduced to avoid flakes

9b0a458

Revert Database.runTransaction runner ordering

0458064

surbhigarg92 reviewed Oct 24, 2024

View reviewed changes

surbhigarg92 approved these changes Oct 24, 2024

View reviewed changes

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

f0b8745

…back response

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

chore(main): release 7.15.0 #2118

Merged

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!

odeke-em commented Oct 23, 2024

Uh oh!

odeke-em commented Oct 23, 2024

Uh oh!

odeke-em commented Oct 23, 2024

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!

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!

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