Skip to content

fix: Skip promise chaining when original promise has additional fields #1618

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.

Sign up for GitHub

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

Closed
Luca Forstner (lforst) wants to merge 4 commits into from
Closed

fix: Skip promise chaining when original promise has additional fields #1618

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60c04ae
Fix more centrally
lforst Mar 26, 2026
49f0f00
Merge remote-tracking branch 'origin/main' into lforst/fix-issue-1591
lforst Mar 27, 2026
91298ee
Also check prototype and fix assertion for older versions
lforst Mar 27, 2026
6eb8bd4
Fix type
lforst Mar 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 14 additions & 33 deletions e2e/scenarios/anthropic-instrumentation/scenario.impl.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,7 @@ const WEATHER_TOOL = {

async function runAnthropicInstrumentationScenario(
Anthropic,
{
decorateClient,
useBetaMessages = true,
useMessagesStreamHelper = true,
} = {},
{ decorateClient, useBetaMessages = true } = {},
) {
const imageBase64 = (
await readFile(new URL("./test-image.png", import.meta.url))
Expand Down Expand Up @@ -123,33 +119,19 @@ async function runAnthropicInstrumentationScenario(
"anthropic-stream-with-response-operation",
"stream-with-response",
async () => {
const stream =
useMessagesStreamHelper === false
? await client.messages.create({
model: ANTHROPIC_MODEL,
max_tokens: 32,
temperature: 0,
stream: true,
messages: [
{
role: "user",
content:
"Count from 1 to 3 and include the words one two three.",
},
],
})
: client.messages.stream({
model: ANTHROPIC_MODEL,
max_tokens: 32,
temperature: 0,
messages: [
{
role: "user",
content:
"Count from 1 to 3 and include the words one two three.",
},
],
});
const stream = client.messages.stream({
model: ANTHROPIC_MODEL,
max_tokens: 32,
temperature: 0,
messages: [
{
role: "user",
content:
"Count from 1 to 3 and include the words one two three.",
},
],
});
await stream.withResponse();
await collectAsync(stream);
},
);
Expand Down Expand Up @@ -251,7 +233,6 @@ export async function runWrappedAnthropicInstrumentation(Anthropic, options) {
export async function runAutoAnthropicInstrumentation(Anthropic, options) {
await runAnthropicInstrumentationScenario(Anthropic, {
...options,
useMessagesStreamHelper: false,
});
}

Expand Down
42 changes: 16 additions & 26 deletions e2e/scenarios/openai-instrumentation/scenario.impl.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,6 @@ async function collectOneAndReturn(stream) {
}
}

async function awaitMaybeWithResponse(request) {
if (typeof request?.withResponse === "function") {
return await request.withResponse();
}

return {
data: await request,
};
}

export async function runOpenAIInstrumentationScenario(options) {
const baseClient = new options.OpenAI({
apiKey: process.env.OPENAI_API_KEY,
Expand All @@ -68,14 +58,14 @@ export async function runOpenAIInstrumentationScenario(options) {
"openai-chat-with-response-operation",
"chat-with-response",
async () => {
await awaitMaybeWithResponse(
client.chat.completions.create({
await client.chat.completions
.create({
model: OPENAI_MODEL,
messages: [{ role: "user", content: "Reply with exactly FOUR." }],
max_tokens: 8,
temperature: 0,
}),
);
})
.withResponse();
},
);

Expand All @@ -97,8 +87,8 @@ export async function runOpenAIInstrumentationScenario(options) {
"openai-stream-with-response-operation",
"stream-with-response",
async () => {
const { data: chatStream } = await awaitMaybeWithResponse(
client.chat.completions.create({
const { data: chatStream } = await client.chat.completions
.create({
model: OPENAI_MODEL,
messages: [
{
Expand All @@ -112,8 +102,8 @@ export async function runOpenAIInstrumentationScenario(options) {
stream_options: {
include_usage: true,
},
}),
);
})
.withResponse();
await collectAsync(chatStream);
},
);
Expand Down Expand Up @@ -210,28 +200,28 @@ export async function runOpenAIInstrumentationScenario(options) {
"openai-responses-with-response-operation",
"responses-with-response",
async () => {
await awaitMaybeWithResponse(
client.responses.create({
await client.responses
.create({
model: OPENAI_MODEL,
input: "What is 2 + 2? Reply with just the number.",
max_output_tokens: 16,
}),
);
})
.withResponse();
},
);

await runOperation(
"openai-responses-create-stream-operation",
"responses-create-stream",
async () => {
const { data: responseStream } = await awaitMaybeWithResponse(
client.responses.create({
const { data: responseStream } = await client.responses
.create({
model: OPENAI_MODEL,
input: "Reply with exactly RESPONSE STREAM.",
max_output_tokens: 16,
stream: true,
}),
);
})
.withResponse();
await collectAsync(responseStream);
},
);
Expand Down
21 changes: 21 additions & 0 deletions js/src/auto-instrumentations/patch-tracing-channel.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,27 @@ describe("patchTracingChannel", () => {
expect(withResponse.response.ok).toBe(true);
});

it("patched tracePromise preserves helper methods on augmented native Promise instances", async () => {
const FakeTCClass = makeUnpatchedTracingChannel();
const channel = new FakeTCClass();
patchTracingChannel(() => channel);

const nativePromise = Promise.resolve("hello") as Promise<string> & {
withResponse: () => Promise<{ data: string; response: { ok: boolean } }>;
};
nativePromise.withResponse = async () => ({
data: await nativePromise,
response: { ok: true },
});

const traced = channel.tracePromise(() => nativePromise, {}, null);
const withResponse = await traced.withResponse();

expect(traced).toBe(nativePromise);
expect(withResponse.data).toBe("hello");
expect(withResponse.response.ok).toBe(true);
});

it("patched tracePromise correctly handles plain async functions", async () => {
const FakeTCClass = makeUnpatchedTracingChannel();
const channel = new FakeTCClass();
Expand Down
10 changes: 8 additions & 2 deletions js/src/auto-instrumentations/patch-tracing-channel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export function patchTracingChannel(
// established by bindStore — required for span context to propagate across awaits.
// PATCHED: inside the callback, use duck-type thenable check instead of
// PromisePrototypeThen, which triggers Symbol.species and breaks Promise subclasses
// like Anthropic's APIPromise that have non-standard constructors.
// like Anthropic's and Openai's APIPromise that have non-standard constructors.
return start.runStores(context, () => {
try {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand All @@ -89,7 +89,13 @@ export function patchTracingChannel(
(typeof result === "object" || typeof result === "function") &&
typeof result.then === "function"
) {
if (result.constructor === Promise) {
if (
// We only want to return the Promise chain when it's an actual
// promise and also doesn't have any additional fields
result.constructor === Promise &&
Object.getOwnPropertyNames(result).length === 0 &&
Object.getOwnPropertySymbols(result).length === 0
Copy link
Copy Markdown
Contributor

@Qard Stephen Belanger (Qard) Mar 26, 2026

Choose a reason for hiding this comment

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

Are these actually necessary? Shouldn't the result.constructor === Promise check already fail for APIPromise?

) {
return result.then(
(res) => {
publishResolved(res);
Expand Down
Loading