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

odeke-em · 2024-10-04T11:22:43Z

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

conventional-commit-lint-gcf · 2024-10-04T11:33:13Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

…al AsyncHooksManager 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

odeke-em · 2024-10-04T11:47:04Z

Kindly help me run the bots @surbhigarg92 @alkatrivedi @harshachinta.

surbhigarg92 · 2024-10-04T11:55:33Z

src/instrument.ts

+/*
+ * This function ensures that async/await functions correctly by
+ * checking if context.active() returns an invalid/unset context
+ * and if so, sets a global AsyncHooksContextManager.
+ */


Suggested change

/*

* This function ensures that async/await functions correctly by

* checking if context.active() returns an invalid/unset context

* and if so, sets a global AsyncHooksContextManager.

*/

/**

* Ensures that an appropriate Context Manager is set to support proper

* context propagation when using async/await, particularly in OpenTelemetry.

*

* OpenTelemetry's context system relies on `context.active()` to retrieve the

* current active context. However, if this returns the default `ROOT_CONTEXT`,

* it indicates that no context has been previously set. Without a valid context,

* async/await calls will fail to propagate context correctly, which can cause

* issues in distributed tracing and telemetry data collection.

*

* This function checks if the active context is invalid (i.e., `ROOT_CONTEXT`).

* If so, it resets the Context Manager by disabling any prior manager and then

* enabling a new instance of `AsyncHooksContextManager` to properly track async

* operations and context propagation.

*

* The goal is to ensure that users' code works seamlessly with OpenTelemetry,

* without requiring them to modify their existing code to manually manage context.

*

* Further details:

* - OpenTelemetry context documentation: https://opentelemetry.io/docs/languages/js/context/#active-context

* - Related issue: https://github.com/googleapis/nodejs-spanner/issues/2146

*/

@odeke-em I generated this documentation using AI, please review once more. But we need a more structured documentation

Some of this AI generated documentation is not correct like

* The goal is to ensure that users' code works seamlessly with OpenTelemetry, * without requiring them to modify their existing code to manually manage context.

it's nothing about manually managing context but setting a global context manager.

@surbhigarg92 the generated AI comments instead just is listing out what the function does but not really why and the purpose of the code which is to check if there was any already set contextManager and if not, set one.

surbhigarg92 · 2024-10-04T11:55:55Z

src/instrument.ts

+    // If no active context was set previously, trace context propagation cannot
+    // work correctly with async/await for OpenTelemetry and they acknowledge
+    // this fact per https://opentelemetry.io/docs/languages/js/context/#active-context
+    // 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://github.com/googleapis/nodejs-spanner/issues/2146


Suggested change

// If no active context was set previously, trace context propagation cannot

// work correctly with async/await for OpenTelemetry and they acknowledge

// this fact per https://opentelemetry.io/docs/languages/js/context/#active-context

// 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://github.com/googleapis/nodejs-spanner/issues/2146

// No active context set, so context propagation won't work with async/await.

// Fix by disabling any prior context manager and setting a new AsyncHooksContextManager.

surbhigarg92 · 2024-10-04T11:57:35Z

observability-test/spanner.ts

@@ -583,3 +616,118 @@ describe('ObservabilityOptions injection and propagation', async () => {
    });
  });
 });
+
+describe('Bug fixes', () => {


Use a better name for this. If you are testing "async/await" use name like Observability tests with async/await

I'd highly encourage keeping such a section for known and fixed regressions with isolated test cases, otherwise eventually we'll just have a pile up of test cases that we don't have a single reference to.

This is the section in which we shall be inserting all regression tests and bug fixes.

I tweaked it, please take a look.

odeke-em · 2024-10-04T22:07:29Z

Kindly help me run the bots @alkatrivedi @surbhigarg92 @harshachinta

…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-05T02:57:16Z

I've instead moved the fix into one PR #2145 because it takes 3+ hours just to run tests, 1+ day for 1 PR code reviews but we don't have much time.

…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 requested review from a team as code owners October 4, 2024 11:22

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

odeke-em changed the title ~~fix (observability): make async/await correctly work by setting initial AsyncHooksManager~~ feat (observability): make async/await correctly work by setting initial AsyncHooksManager Oct 4, 2024

odeke-em force-pushed the observability-detect-and-set-initial-asyncHooksManager branch from 2c001c6 to 857ac65 Compare October 4, 2024 11:33

odeke-em force-pushed the observability-detect-and-set-initial-asyncHooksManager branch from 857ac65 to 44bc1b7 Compare October 4, 2024 11:34

odeke-em changed the title ~~feat (observability): make async/await correctly work by setting initial AsyncHooksManager~~ fix: (observability) make async/await correctly work by setting initial AsyncHooksManager Oct 4, 2024

surbhigarg92 reviewed Oct 4, 2024

View reviewed changes

Tweak documentation and test name

9d04d6d

odeke-em requested a review from surbhigarg92 October 4, 2024 13:12

odeke-em mentioned this pull request Oct 5, 2024

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

Open

odeke-em closed this Oct 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

odeke-em commented Oct 4, 2024

conventional-commit-lint-gcf bot commented Oct 4, 2024

odeke-em commented Oct 4, 2024

surbhigarg92 Oct 4, 2024

surbhigarg92 Oct 4, 2024

odeke-em Oct 4, 2024

odeke-em Oct 4, 2024

surbhigarg92 Oct 4, 2024

surbhigarg92 Oct 4, 2024

odeke-em Oct 4, 2024

odeke-em Oct 4, 2024

odeke-em Oct 4, 2024

odeke-em commented Oct 4, 2024

odeke-em commented Oct 5, 2024

-/*
- * This function ensures that async/await functions correctly by
- * checking if context.active() returns an invalid/unset context
- * and if so, sets a global AsyncHooksContextManager.
- */
+/**
+ * Ensures that an appropriate Context Manager is set to support proper
+ * context propagation when using async/await, particularly in OpenTelemetry.
+ *
+ * OpenTelemetry's context system relies on `context.active()` to retrieve the
+ * current active context. However, if this returns the default `ROOT_CONTEXT`,
+ * it indicates that no context has been previously set. Without a valid context,
+ * async/await calls will fail to propagate context correctly, which can cause
+ * issues in distributed tracing and telemetry data collection.
+ *
+ * This function checks if the active context is invalid (i.e., `ROOT_CONTEXT`).
+ * If so, it resets the Context Manager by disabling any prior manager and then
+ * enabling a new instance of `AsyncHooksContextManager` to properly track async
+ * operations and context propagation.
+ *
+ * The goal is to ensure that users' code works seamlessly with OpenTelemetry,
+ * without requiring them to modify their existing code to manually manage context.
+ *
+ * Further details:
+ * - OpenTelemetry context documentation: https://opentelemetry.io/docs/languages/js/context/#active-context
+ * - Related issue: https://github.com/googleapis/nodejs-spanner/issues/2146
+ */

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

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

Conversation

odeke-em commented Oct 4, 2024

conventional-commit-lint-gcf bot commented Oct 4, 2024

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

odeke-em commented Oct 4, 2024

odeke-em commented Oct 5, 2024