Skip to content
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

observability: if an async context manager has not been enabled, make sure to register one #2146

Closed
1 task
odeke-em opened this issue Oct 4, 2024 · 0 comments · Fixed by #2145
Closed
1 task
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@odeke-em
Copy link
Contributor

odeke-em commented Oct 4, 2024

Reported offline by @alkatrivedi, whereby their code didn't involve creating a new traces at all but was using this instrumented package and their code used async/await yet spans that are supposed to be nested were disjoint.

OpenTelemetry-JS acknowledges that for code to work correctly and for context to be correctly propagated (as a replacement for threadlocal storage) a context manager must be enabled beforehand https://opentelemetry.io/docs/languages/js/context/

Customers should never have to think about these reasons and do it themselves, instead our package should automatically handle this scenario so that customers can transparently keep their code without any needs to edit

Task

  • Check for if a prior context manager was enabled and if not, create oen like this ideally in src/instrument.ts
const {AsyncHooksContextManager} = require('@opentelemetry/context-async-hooks');
const {context} = require('@opentelemetry/api');
const contextManager = new AsyncHooksContextManager();
contextManager.enable();
context.setGlobalContextManager(contextManager);

along with regression tests for it.

@odeke-em odeke-em added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 4, 2024
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/nodejs-spanner API. label Oct 4, 2024
odeke-em added a commit to odeke-em/nodejs-spanner that referenced this issue Oct 4, 2024
…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 added a commit to odeke-em/nodejs-spanner that referenced this issue Oct 4, 2024
…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 added a commit to odeke-em/nodejs-spanner that referenced this issue Oct 4, 2024
…ial 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 added a commit to odeke-em/nodejs-spanner that referenced this issue Oct 4, 2024
…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 added a commit to odeke-em/nodejs-spanner that referenced this issue Oct 5, 2024
…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 added a commit to odeke-em/nodejs-spanner that referenced this issue Oct 5, 2024
…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 added a commit to odeke-em/nodejs-spanner that referenced this issue Oct 5, 2024
…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 added a commit to odeke-em/nodejs-spanner that referenced this issue Oct 7, 2024
…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 added a commit to odeke-em/nodejs-spanner that referenced this issue Oct 8, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
1 participant