-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
🤖 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 -- conventional-commit-lint bot |
2c001c6
to
857ac65
Compare
…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
857ac65
to
44bc1b7
Compare
Kindly help me run the bots @surbhigarg92 @alkatrivedi @harshachinta. |
src/instrument.ts
Outdated
/* | ||
* 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. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* | |
* 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 | |
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@odeke-em I generated this documentation using AI, please review once more. But we need a more structured documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
src/instrument.ts
Outdated
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. | |
observability-test/spanner.ts
Outdated
@@ -583,3 +616,118 @@ describe('ObservabilityOptions injection and propagation', async () => { | |||
}); | |||
}); | |||
}); | |||
|
|||
describe('Bug fixes', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a better name for this. If you are testing "async/await" use name like Observability tests with async/await
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the section in which we shall be inserting all regression tests and bug fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked it, please take a look.
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
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
…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 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 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
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