feat: (observability) trace Database.batchCreateSessions + SessionPool.createSessions #2145

odeke-em · 2024-10-04T03:02:25Z

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

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

src/session-pool.ts

surbhigarg92 · 2024-10-04T05:36:03Z

Can you also add record session leaks in opentelemetry traces.

Add the this._getLeaks() in traces, also

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

…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

odeke-em · 2024-10-08T13:46:43Z

And rebased from main, good to get to bots testing and review updates @alkatrivedi @surbhigarg92!

observability-test/database.ts

src/database.ts

surbhigarg92 · 2024-10-09T11:24:32Z

src/session-pool.ts

@@ -485,6 +491,9 @@ export class SessionPool extends EventEmitter implements SessionPoolInterface {
 });

 this._traces = new Map();
+ if (!this._observabilityOptions) {


Why do we need this if check. We should simply override the observability option ?

Not really, I was being cautious in case someone had passed in multiple observability configurations one for the explicitly created SessionPool and then one for the Database separately. It doesn't hurt to be conservative at the beginning before these libraries have been put into heavy usage.

But we don't allow customers to pass multiple observability configurations right ? They should be able to set it only once via spannerOptions.

We also modified the _observabilityOptions object to add _ to represent private variable.

surbhigarg92 · 2024-10-09T11:24:45Z

src/session-pool.ts

- // will return at most 100 at a time, hence the need for a while loop.
- while (amount > 0) {
- let sessions: Session[] | null = null;
+ if (!this.database._observabilityOptions) {


Why do we need to set. the observability option here again ? We should be doing it in constructor ?

There was some test case in which the observability options for database weren't set despite having been passed into the Spanner constructor.

Then we should find a way to fix those test cases and not add the code here again.

src/session-pool.ts

observability-test/spanner.ts

surbhigarg92 · 2024-10-09T11:40:53Z

observability-test/spanner.ts


- done();
+describe('Regression tests for fixed bugs', () => {


use a better name to describe the test block. Maybe something like E2E traces with async/await ?

But they are really regression tests, wouldn't E2E be even more obscure? Regression to catch any future bugs given an exact use case per bug.

Also btw within this section I've included the callback variant of that bug reproducer

observability-test/spanner.ts

…ready happened before running

surbhigarg92 · 2024-10-10T06:29:02Z

observability-test/spanner.ts

- // See https:/googleapis/nodejs-spanner/issues/2146.
- const traceExporter = new InMemorySpanExporter();
- const provider = new NodeTracerProvider({
+describe('Regression tests for fixed bugs', async () => {


We can not have test block name as 'Regression tests for fixed bugs' . Block should tell what the scope of testing is.
Create two separate block. One is describe('async/await', another describe('callback',

I think I'll defer to you then for the naming as you are the maintainer; this because for other projects we usually have explicit sections for regression tests where it is highly encouraged that you post the reproducers. What name would you like for this?

surbhigarg92 · 2024-10-10T06:34:04Z

src/instrument.ts

+ // but we shouldn't make our customers have to invasively edit their code
+ // nor should they be burdened about these facts, their code should JUST work.
+ // Please see https:/googleapis/nodejs-spanner/issues/2146


I think we should remove this code. As we have added the opentelemetry link which should be enough. The bug will be anyway linked so maintainers would be able to track it using the code blame if required.

surbhigarg92 · 2024-10-10T06:36:06Z

src/session-pool.ts

@@ -485,6 +491,9 @@ export class SessionPool extends EventEmitter implements SessionPoolInterface {
 });

 this._traces = new Map();
+ if (!this._observabilityOptions) {


But we don't allow customers to pass multiple observability configurations right ? They should be able to set it only once via spannerOptions.

We also modified the _observabilityOptions object to add _ to represent private variable.

surbhigarg92 · 2024-10-10T06:37:07Z

src/session-pool.ts

- // will return at most 100 at a time, hence the need for a while loop.
- while (amount > 0) {
- let sessions: Session[] | null = null;
+ if (!this.database._observabilityOptions) {


Then we should find a way to fix those test cases and not add the code here again.

surbhigarg92 · 2024-10-10T07:03:43Z

observability-test/session-pool.ts

+ InMemorySpanExporter,
+} = require('@opentelemetry/sdk-trace-node');
+// eslint-disable-next-line n/no-extraneous-require
+const {SimpleSpanProcessor} = require('@opentelemetry/sdk-trace-base');


Why don't we add @opentelemetry/sdk-trace-base as a test dependency and remove this lint from everywhere in code

@surbhigarg92 it is added in devDependencies

then why do we need to add this eslint suppress ?

Without it we were getting build failures, it's basically the template for the tests in observability-test from long when we started sending in code changes. I really didn't dig in as it required so much time going through and figuring the build system's problems and it wasn't worth the hours sweating on.

…pendency

odeke-em requested review from a team as code owners October 4, 2024 03:02

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

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

feat(observability): trace Transaction

1050d50

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

surbhigarg92 reviewed Oct 4, 2024

View reviewed changes

src/session-pool.ts Outdated Show resolved Hide resolved

odeke-em requested a review from surbhigarg92 October 4, 2024 07:32

odeke-em force-pushed the trace-SessionPool.createSessions+Database.createBatchSesions branch 3 times, most recently from 692a62c to 3d6aaf2 Compare October 4, 2024 07:38

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

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

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

odeke-em force-pushed the trace-SessionPool.createSessions+Database.createBatchSesions branch from 3d6aaf2 to e600592 Compare October 5, 2024 02:54

odeke-em mentioned this pull request Oct 5, 2024

fix: (observability) make async/await correctly work by setting initial AsyncHooksManager #2147

Closed

odeke-em force-pushed the trace-SessionPool.createSessions+Database.createBatchSesions branch 3 times, most recently from 0e5e067 to 385d5c8 Compare October 7, 2024 02:24

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

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

feat(observability): trace Transaction

8dc1243

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

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

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-SessionPool.createSessions+Database.createBatchSesions branch from 385d5c8 to 67d5fbe Compare October 8, 2024 13:46

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

alkatrivedi reviewed Oct 9, 2024

View reviewed changes

observability-test/database.ts Show resolved Hide resolved

surbhigarg92 reviewed Oct 9, 2024

View reviewed changes

Ensure span.end on SessionPool._createSession exception + tests

21fe14a

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

observability-test: deflake tests by ensuring SessionPool creation al…

6c879ec

…ready happened before running

odeke-em force-pushed the trace-SessionPool.createSessions+Database.createBatchSesions branch 2 times, most recently from bcac385 to 1dfcfcf Compare October 9, 2024 17:43

odeke-em requested review from surbhigarg92 and alkatrivedi October 9, 2024 17:46

Add callback variant of regression test

bb44c62

odeke-em force-pushed the trace-SessionPool.createSessions+Database.createBatchSesions branch from 1dfcfcf to bb44c62 Compare October 9, 2024 17:49

Test for span status for SessionPool._createSessions

1c36d39

odeke-em force-pushed the trace-SessionPool.createSessions+Database.createBatchSesions branch from 0fc1258 to 1c36d39 Compare October 10, 2024 06:13

surbhigarg92 reviewed Oct 10, 2024

View reviewed changes

Apply maintainer Surbhi's suggestions

09a5195

odeke-em requested a review from surbhigarg92 October 10, 2024 07:09

surbhigarg92 approved these changes Oct 10, 2024

View reviewed changes

package.json: promote @opentelemetry/context-async-hooks to a main de…

75a5d48

…pendency

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

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

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

surbhigarg92 added automerge Merge the pull request once unit tests and other checks pass. owlbot:run Add this label to trigger the Owlbot post processor. labels Oct 10, 2024

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

gcf-merge-on-green bot merged commit f489c94 into googleapis:main Oct 10, 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 10, 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 Database.batchCreateSessions + SessionPool.createSessions #2145

feat: (observability) trace Database.batchCreateSessions + SessionPool.createSessions #2145

odeke-em commented Oct 4, 2024 •

edited

Loading

surbhigarg92 commented Oct 4, 2024

odeke-em commented Oct 8, 2024

surbhigarg92 Oct 9, 2024

odeke-em Oct 9, 2024

surbhigarg92 Oct 10, 2024

surbhigarg92 Oct 9, 2024

odeke-em Oct 9, 2024

surbhigarg92 Oct 10, 2024

surbhigarg92 Oct 9, 2024

odeke-em Oct 9, 2024

odeke-em Oct 10, 2024

surbhigarg92 Oct 10, 2024 •

edited

Loading

odeke-em Oct 10, 2024

surbhigarg92 Oct 10, 2024

surbhigarg92 Oct 10, 2024

surbhigarg92 Oct 10, 2024

surbhigarg92 Oct 10, 2024

odeke-em Oct 10, 2024

surbhigarg92 Oct 10, 2024

odeke-em Oct 10, 2024

feat: (observability) trace Database.batchCreateSessions + SessionPool.createSessions #2145

feat: (observability) trace Database.batchCreateSessions + SessionPool.createSessions #2145

Conversation

odeke-em commented Oct 4, 2024 • edited Loading

surbhigarg92 commented Oct 4, 2024

odeke-em commented Oct 8, 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

surbhigarg92 Oct 10, 2024 • edited Loading

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

edited

Loading

surbhigarg92 Oct 10, 2024 •

edited

Loading