-
Notifications
You must be signed in to change notification settings - Fork 421
fix: Updated openai instrumentation to properly handle streaming when stream_options.include_usage is set
#3494
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,7 +143,7 @@ test('responses.create', async (t) => { | |
| }) | ||
|
|
||
| const chatSummary = events.filter(([{ type }]) => type === 'LlmChatCompletionSummary')[0] | ||
| assertChatCompletionSummary({ tx, model, chatSummary, tokenUsage: true }) | ||
| assertChatCompletionSummary({ tx, model, chatSummary }) | ||
|
|
||
| tx.end() | ||
| end() | ||
|
|
@@ -174,7 +174,7 @@ test('responses.create', async (t) => { | |
| }) | ||
|
|
||
| const chatSummary = events.filter(([{ type }]) => type === 'LlmChatCompletionSummary')[0] | ||
| assertChatCompletionSummary({ tx, model, chatSummary, tokenUsage: true, singleInput: true }) | ||
| assertChatCompletionSummary({ tx, model, chatSummary, singleInput: true }) | ||
|
|
||
| tx.end() | ||
| end() | ||
|
|
@@ -318,8 +318,7 @@ test('responses.create', async (t) => { | |
| const stream = await client.responses.create({ | ||
| stream: true, | ||
| input: content, | ||
| model: 'gpt-4', | ||
| stream_options: { include_usage: true } | ||
| model: 'gpt-4' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following up with the previous comment, are these tests dependent on the GPT version (if I remember correctly, I think not)? If not, the mocks and tests should just support 1 model id for simplicity
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the model is not taken into account from the mock server. places where the model is stored as a variable is for assertions in the LlmCompletion* events
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, then we could just standardize them e.g. replacing |
||
| }) | ||
|
|
||
| let chunk = {} | ||
|
|
@@ -347,11 +346,11 @@ test('responses.create', async (t) => { | |
| const { client, agent } = t.nr | ||
| helper.runInTransaction(agent, async (tx) => { | ||
| const content = 'Streamed response' | ||
| const model = 'gpt-4-0613' | ||
| const stream = await client.responses.create({ | ||
| stream: true, | ||
| input: [{ role: 'user', content }, { role: 'user', content: 'What does 1 plus 1 equal?' }], | ||
| model: 'gpt-4', | ||
| stream_options: { include_usage: true } | ||
| model, | ||
| }) | ||
|
|
||
| let chunk = {} | ||
|
|
@@ -366,10 +365,12 @@ test('responses.create', async (t) => { | |
| tx, | ||
| chatMsgs, | ||
| id: 'resp_684886977be881928c9db234e14ae7d80f8976796514dff9', | ||
| model: 'gpt-4-0613', | ||
| model, | ||
| resContent: res, | ||
| reqContent: content | ||
| }) | ||
| const chatSummary = events.filter(([{ type }]) => type === 'LlmChatCompletionSummary')[0] | ||
| assertChatCompletionSummary({ tx, model, chatSummary, streaming: true }) | ||
|
|
||
| tx.end() | ||
| end() | ||
|
|
@@ -378,23 +379,27 @@ test('responses.create', async (t) => { | |
|
|
||
| await t.test('should call the tokenCountCallback in streaming', (t, end) => { | ||
| const { client, agent } = t.nr | ||
| const model = 'gpt-4-0613' | ||
| const promptContent = 'Streamed response' | ||
| const promptContent2 = 'What does 1 plus 1 equal?' | ||
| const promptTokens = 53 | ||
| const completionTokens = 11 | ||
| const res = 'Test stream' | ||
| const api = helper.getAgentApi() | ||
| // swap the token counts | ||
| function cb(model, content) { | ||
| // could be gpt-4 or gpt-4-0613 | ||
| assert.ok(model === 'gpt-4' || model === 'gpt-4-0613', 'should be gpt-4 or gpt-4-0613') | ||
| if (content === promptContent + ' ' + promptContent2) { | ||
| return 53 | ||
| return promptTokens | ||
| } else if (content === res) { | ||
| return 11 | ||
| return completionTokens | ||
| } | ||
| } | ||
| api.setLlmTokenCountCallback(cb) | ||
| helper.runInTransaction(agent, async (tx) => { | ||
| const stream = await client.responses.create({ | ||
| model: 'gpt-4', | ||
| model, | ||
| input: [ | ||
| { role: 'user', content: promptContent }, | ||
| { role: 'user', content: promptContent2 } | ||
|
|
@@ -410,14 +415,15 @@ test('responses.create', async (t) => { | |
| const events = agent.customEventAggregator.events.toArray() | ||
| const chatMsgs = events.filter(([{ type }]) => type === 'LlmChatCompletionMessage') | ||
| assertChatCompletionMessages({ | ||
| tokenUsage: true, | ||
| tx, | ||
| chatMsgs, | ||
| id: 'resp_684886977be881928c9db234e14ae7d80f8976796514dff9', | ||
| model: 'gpt-4-0613', | ||
| model, | ||
| resContent: res, | ||
| reqContent: promptContent | ||
| }) | ||
| const chatSummary = events.filter(([{ type }]) => type === 'LlmChatCompletionSummary')[0] | ||
| assertChatCompletionSummary({ tx, model, chatSummary, streaming: true, promptTokens, completionTokens }) | ||
|
|
||
| tx.end() | ||
| end() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit, but I feel like we should store these model ids off as constants at the top of the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can but rather do this as a follow up. These 3 PRs around tokens fix bugs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, just wanted to point it out