Skip to content

test: stop using the obsolete AsyncHooksContextManager in tests #2864

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

Merged

Conversation

trentm
Copy link
Contributor

@trentm trentm commented May 28, 2025

Since support for Node.js <14.8.0 was dropped, the modern
AsyncLocalStorageContextManager can always be used. This was done
for runtime code in open-telemetry/opentelemetry-js#4341
This PR does this for tests that were explicitly setting a
context manager.

Since support for Node.js <14.8.0 was dropped, the modern
AsyncLocalStorageContextManager can always be used. This was done
for runtime code in open-telemetry/opentelemetry-js#4341
This PR does this for tests that were explicitly setting a
context manager.
Copy link

codecov bot commented May 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.69%. Comparing base (6f65523) to head (f38ad20).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2864      +/-   ##
==========================================
+ Coverage   89.65%   89.69%   +0.04%     
==========================================
  Files         185      185              
  Lines        9034     9034              
  Branches     1852     1852              
==========================================
+ Hits         8099     8103       +4     
+ Misses        935      931       -4     

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

trentm added 2 commits May 28, 2025 14:07
…pear on some spans where they did not before, presumably because AsyncLocalStorage is doing a better job tracing the async context
assertSingleSpan('cassandra-driver.execute', undefined, undefined, {
[SEMATTRS_NET_PEER_NAME]: cassandraContactPoint,
[SEMATTRS_NET_PEER_PORT]: 9042,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review note: the switch to AsyncLocalStorage for context management results in some spans getting net.peer.{name,port} being applied where it was not before. Presumably this is because AsyncLocalStorage is doing a better job following the async-context and this code block now gets triggered.

Copy link
Contributor

This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature.
Are you familiar with this package? Consider becoming a component owner.

@trentm trentm marked this pull request as ready for review May 29, 2025 16:17
@trentm trentm requested a review from a team as a code owner May 29, 2025 16:17
@trentm trentm merged commit 2ac08ff into open-telemetry:main May 30, 2025
23 checks passed
@trentm trentm deleted the trentm-bye-bye-AsyncHooksContextManager branch May 30, 2025 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment