fix: Skip promise chaining when original promise has additional fields

[Luca Forstner (lforst)]

OpenAI and Anthropic SDK functions sometimes return Promises (having Promise prototype) with additional fields. Whenever we wrap these Promises with a chain (.then()) the return value will not have the additional field, breaking API contracts.

To mitigate we have previously introduced a check to compare the prototype to Promise to make sure that we only chain actual promises, however that was only enough for OpenAI. Anthropic seems to return actual Promises with additional fields so we gotta add an additional check that checks for additional ownproperties.

[Stephen Belanger (Qard)]

The traceSyncResultChannel/patchOpenAIAPIPromiseResult thing seems incredibly convoluted. Why can we not just use tracePromise for those? I'm not understanding why we're reinventing the promise patching already provided by that. 🤔

[Luca Forstner (lforst)]

The traceSyncResultChannel/patchOpenAIAPIPromiseResult thing seems incredibly convoluted. Why can we not just use tracePromise for those? I'm not understanding why we're reinventing the promise patching already provided by that. 🤔

Stephen Belanger (@Qard) I personally don't find it too convoluted. Would you mind explaining what just using tracePromise would do. We still have to extract a bunch of data and handle async iterators and such. Also we are dealing with funky extended promises again here so that's an added complication.

[Stephen Belanger (Qard)]

We already addressed the thenables problem though, so that should be a non-issue. As for the async iterator thing, is the sync result itself directly an async iterator? Or is that wrapped in a promise? I'm working on some improvements to tracing channel in Node.js core to address the lack of async iterator support. It would be good if we try to align with that. Namely, we should still be producing the asyncStart and asyncEnd around the overall boundaries, and should have boundary events around each yield too ideally.

The benefit of conforming to the diagnostics_channel model is that you get discrete event pairs which can be understood more generically rather than having all this sort of code which ties deeply into the implementation details of the code and is therefore brittle to changes to the API surface. If the code changes with direct patching like this the patches could break. If the code changes with diagnostics_channel either the channel publishing changes with it or the channels simply don't emit those events anymore and so nothing happens.

What I had done for these things previously was just patch the received async iterators in the diagnostics_channel handlers we did have provided as we do not have the async iterator mechanisms yet. But we should still express the full async lifecycle though because that's what indicates when to stop a corresponding span, right? It's supposed to be the case that we can simply use the diagnostics_channel events directly for starting/stopping spans and then we can augment that with additional channel events to capture other details like errors or logs.

While tracePromise might be insufficient for the async iterator part, I'm not seeing why it's not at least usable for the rest of that and we can just deviate minimally for only the async iterator need?

[Luca Forstner (lforst)]

So the thing is the OpenAI SDK returns APIPromise for a bunch of api which can be used in two ways:

const p = client.chat.completions.create({
  stream: true / false
});

// depending on stream === true or false:

// stream === true: async iterator, stream === false: parsed data
const data = await p; 

// stream === true: { data: async iterator, response: http response}
// stream === false: { data: parsed data, response: http response }
const full = await p.withResponse(); 

The benefit of conforming to the diagnostics_channel model is that you get discrete event pairs which can be understood more generically rather than having all this sort of code which ties deeply into the implementation details of the code and is therefore brittle to changes to the API surface. If the code changes with direct patching like this the patches could break. If the code changes with diagnostics_channel either the channel publishing changes with it or the channels simply don't emit those events anymore and so nothing happens.

I guess we can do what I believe you're suggesting and use tracePromise to wrap the response promises. However I struggle to understand how we will get around patching the results (?) to instrument withResponse? Also I think the point about breaking is a bit moot because from what I can tell the patching is more or less transparent.

I argue, creating more DCs does nothing but create more indirection because now we are emitting tracing channels for which we again need to register handlers which will need data on on what exactly we are patching - on top of which I don't think we will get around patching things. Imo tracing channels are an entry point for us trying to "get where we need to be" and nothing more. We gain nothing from emitting more of them. Imo it's much clearer to just do all of the patching in one place. I don't think tracing channels are a powerful enough API for what you seem to wanting to be doing but maybe I am wrong.

Maybe I am misunderstanding, in the case of which you can maybe open a PR for an alternative implementation? Maybe that helps illustrate your case better?

[Luca Forstner (lforst)]

Stephen Belanger (@Qard) I completely nuked this fix because I realized we can address it somewhere else. Lmk what you think.

[Stephen Belanger (Qard)]

Generally LGTM, but are you certain those property name/symbol checks are actually necessary? An instance of APIPromise should have p.constructor of APIPromise should it not?

[Stephen Belanger (Qard)]

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