fix(otel): propagate promise rejection from startActiveSpan async callback - #2757
Open
Hector Hernandez (hectorhdzg) wants to merge 3 commits into
Open
Conversation
…lback (microsoft#2749) startActiveSpan swallowed rejections from async callbacks: the doAwait reject handler set the span status but never re-threw the reason, so the returned promise resolved with undefined instead of rejecting. Re-throw the rejection reason so the returned promise rejects, matching the synchronous catch path. Adds a regression test covering rejection propagation, span end, and active-span restoration.
Copilot started reviewing on behalf of
Hector Hernandez (hectorhdzg)
August 13, 2026 22:40
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes OpenTelemetry Tracer.startActiveSpan() so that promise rejections from async callbacks are correctly propagated to the caller (matching the synchronous exception path), instead of being swallowed and returning a resolved undefined. This aligns behavior with expected async error handling and addresses issue #2749.
Changes:
- Re-throw the rejection reason inside the
doAwaitrejection handler so the returned promise rejects. - Add a regression unit test validating rejection propagation, span end behavior, and active-span restoration.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| shared/AppInsightsCore/src/otel/api/trace/utils.ts | Ensures startActiveSpan propagates async rejection reasons by re-throwing in the promise rejection handler. |
| shared/AppInsightsCore/Tests/Unit/src/trace/span.Tests.ts | Adds regression coverage for async rejection propagation and related span lifecycle/active-span behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…led rejection The async branch of startActiveSpan passed span.end() as doAwait's finallyFn. ts-async's doAwait discards the return of its internal doFinally(result, finallyFn), so on rejection the internal result.finally() promise rejected and was never handled, surfacing as a global unhandled rejection. End the span directly in both the resolve and reject handlers and drop the finallyFn. Test rewritten to the established createRejectedPromise + direct doAwait pattern.
The 'actual sampling rate should fall into 5% error range' test used 1000 random ids against a +-5% threshold (~3 sigma), so occasional newId() distributions crossed it and failed CI intermittently. Increase the sample size to 10000 so the statistical margin is ~10 sigma (effectively never fails) while preserving the random-input intent. Also fix the stale '+-2%' assertion message to reflect the actual errorRange.
Jackson Weber (JacksonWeber)
approved these changes
Aug 14, 2026
Radhika Gupta (rads-1996)
approved these changes
Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
startActiveSpan swallowed rejections from async callbacks: the doAwait reject handler set the span status but never re-threw the reason, so the returned promise resolved with undefined instead of rejecting.
Re-throw the rejection reason so the returned promise rejects, matching the synchronous catch path. Adds a regression test covering rejection propagation, span end, and active-span restoration.
Fixes #2749