feat(observability): trace Transaction #2122

odeke-em · 2024-09-22T12:36:13Z

This change adds observability tracing for
Transaction along with tests.

Updates #2079
Built from PR #2087
Updates #2114

…l.createSessions This change adds tracing for Database.batchCreateSessions as well as SessionPool.createSessions which was raised as a big need. This change is a premise to finishing up tracing Transaction. While here, also folded in the async/await fix to avoid day+ long code review lag and then 3+ hours just to run tests per PR: OpenTelemetry cannot work correctly for async/await if there isn't a set AsyncHooksManager, but we should not burden our customers with this type of specialist knowledge, their code should just work and this change performs such a check. Later on we shall file a feature request with the OpenTelemetry-JS API group to give us a hook to detect if we've got a live asyncHooksManager instead of this mandatory comparison to ROOT_CONTEXT each time. Fixes googleapis#2146 Updates googleapis#2079 Spun out of PR googleapis#2122 Supersedes PR googleapis#2147

src/transaction.ts

surbhigarg92 · 2024-10-08T06:25:32Z

src/transaction.ts

- if (err) {
- callback!(err, resp);
- return;
+ const q = {opts: this._observabilityOptions};


Also can't we create the observabilityOptions object once in the constructor and use that everywhere, instead of creating it in every method.

Or atleast create a basic object which can be used everywhere and extend it to the method where its required.

Sometimes we have to add some attributes to it, sometimes we have newly injected values of this._observabilityOptions.

sometimes we have newly injected values of
When will we be having that. IMU this can only be set at the time of spanner creation via spanner options

If we define the basic object in constructor then we can extend that object whereever we need using "Object.assign" . WDYT ?

I'd say let's keep it as is because as I mentioned there are instances where we copy over _observabilityOptions to an already existing object so keeping it in the constructor is non-kosher.

src/transaction.ts

surbhigarg92 · 2024-10-08T07:32:20Z

src/transaction.ts

+ span.end();
+ });
+
+ if (!prs) {


I don't understand what is this check for ?

There are instances where we have an EventEmitter returned that I found in tests, not being returned by the type assertion to PartialResultStream.

Can you please link the test, I am not able to get the reason for this check.

@odeke-em Please see this

This check was being defensive from some odd condition in earlier changes but now it doesn't fail. However, the others that check is instanceof Stream are valid.

Removed, thank you @surbhigarg92!

@surbhigarg92 actually I found where the problem was and it shows in the test

Transaction Snapshot createReadStream should use the transaction id if present: TypeError [ERR_INVALID_ARG_TYPE]: The "stream" argument must be an instance of ReadableStream, WritableStream, or Stream. Received undefined

notice the

TypeError [ERR_INVALID_ARG_TYPE]: The "stream" argument must be an instance of ReadableStream, WritableStream, or Stream. Received undefined

particularly for
argument must be an instance of ReadableStream, WritableStream, or Stream. Received undefined

src/transaction.ts

observability-test/spanner.ts

This change adds observability tracing for Transaction along with tests. Updates googleapis#2079 Updates googleapis#2114 Requires PR googleapis#2145.

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

Thank you very much for the reviews and approval @surbhigarg92!

…l.createSessions This change adds tracing for Database.batchCreateSessions as well as SessionPool.createSessions which was raised as a big need. This change is a premise to finishing up tracing Transaction. While here, also folded in the async/await fix to avoid day+ long code review lag and then 3+ hours just to run tests per PR: OpenTelemetry cannot work correctly for async/await if there isn't a set AsyncHooksManager, but we should not burden our customers with this type of specialist knowledge, their code should just work and this change performs such a check. Later on we shall file a feature request with the OpenTelemetry-JS API group to give us a hook to detect if we've got a live asyncHooksManager instead of this mandatory comparison to ROOT_CONTEXT each time. Fixes googleapis#2146 Updates googleapis#2079 Spun out of PR googleapis#2122 Supersedes PR googleapis#2147

…l.createSessions (#2145) This change adds tracing for Database.batchCreateSessions as well as SessionPool.createSessions which was raised as a big need. This change is a premise to finishing up tracing Transaction. While here, also folded in the async/await fix to avoid day+ long code review lag and then 3+ hours just to run tests per PR: OpenTelemetry cannot work correctly for async/await if there isn't a set AsyncHooksManager, but we should not burden our customers with this type of specialist knowledge, their code should just work and this change performs such a check. Later on we shall file a feature request with the OpenTelemetry-JS API group to give us a hook to detect if we've got a live asyncHooksManager instead of this mandatory comparison to ROOT_CONTEXT each time. Fixes #2146 Updates #2079 Spun out of PR #2122 Supersedes PR #2147

product-auto-label bot added size: xl Pull request size is extra large. api: spanner Issues related to the googleapis/nodejs-spanner API. labels Sep 22, 2024

odeke-em force-pushed the trace-Transaction branch 13 times, most recently from 9a48feb to a4e56f3 Compare September 22, 2024 13:29

product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Sep 22, 2024

odeke-em force-pushed the trace-Transaction branch 5 times, most recently from b4217d8 to afa5873 Compare September 23, 2024 07:24

odeke-em force-pushed the trace-Transaction branch 3 times, most recently from d8fe92b to 838225e Compare October 1, 2024 08:23

odeke-em marked this pull request as ready for review October 1, 2024 08:23

odeke-em requested review from a team as code owners October 1, 2024 08:23

odeke-em force-pushed the trace-Transaction branch from 838225e to 9fa7878 Compare October 1, 2024 08:26

product-auto-label bot removed the size: l Pull request size is large. label Oct 1, 2024

odeke-em force-pushed the trace-Transaction branch from 1050d50 to 8dc1243 Compare October 7, 2024 09:17

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 7, 2024

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

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

surbhigarg92 reviewed Oct 8, 2024

View reviewed changes

odeke-em requested a review from surbhigarg92 October 8, 2024 08:43

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 8, 2024

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

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

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

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

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

surbhigarg92 approved these changes Oct 8, 2024

View reviewed changes

feat(observability): trace Transaction

69c8541

This change adds observability tracing for Transaction along with tests. Updates googleapis#2079 Updates googleapis#2114 Requires PR googleapis#2145.

odeke-em force-pushed the trace-Transaction branch from 3d5fa6c to 69c8541 Compare October 8, 2024 09:51

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

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

surbhigarg92 added the automerge Merge the pull request once unit tests and other checks pass. label Oct 8, 2024

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

gcf-merge-on-green bot merged commit a464bdb into googleapis:main Oct 8, 2024
16 checks passed

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 8, 2024

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

chore(main): release 7.15.0 #2118

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(observability): trace Transaction #2122

feat(observability): trace Transaction #2122

odeke-em commented Sep 22, 2024

surbhigarg92 Oct 8, 2024

surbhigarg92 Oct 8, 2024

odeke-em Oct 8, 2024

surbhigarg92 Oct 8, 2024

odeke-em Oct 8, 2024

surbhigarg92 Oct 8, 2024

odeke-em Oct 8, 2024

surbhigarg92 Oct 8, 2024

surbhigarg92 Oct 8, 2024

odeke-em Oct 8, 2024

odeke-em Oct 8, 2024

odeke-em Oct 8, 2024

odeke-em commented Oct 8, 2024

feat(observability): trace Transaction #2122

feat(observability): trace Transaction #2122

Conversation

odeke-em commented Sep 22, 2024

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

odeke-em commented Oct 8, 2024