Skip to content
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

feat: Added instrumentation for openai chat completion creation #1862

Merged
merged 6 commits into from Nov 15, 2023

Conversation

bizob2828
Copy link
Member

@bizob2828 bizob2828 commented Nov 13, 2023

Description

This PR adds instrumentation for chat completion create. It adds the relevant span and the associated LLM events: n chat completion messages, and 1 chat completion summary.

Note: Error handling has been deferred to #1845 as well as stream based chat completions(#1844).

Aside from instrumentation openai.Chat.Completions.prototype.create, I had to wrap openai.prototype.makeRequest to decorate the active segment with request headers and api key to be used when creating the LLM events. As you can see they are cleaned up before returning the result to the user. I also had to enhance shim to pass in the active segment as the last arg, this is because once a promise resolves the active segment is its parent. We need the active segment that was being recorded to decorate the LLM events.

How to Test

npm run versioned:internal openai

node test/unit/instrumentation/openai.test.js

If you want to use a sample app you can use this as the basis of it:

const newrelic = require('newrelic')
const OpenAI = require('openai')
const openai = new OpenAI({
  apiKey: process.env.OPENAI_API_KEY
})

async function main() {
  newrelic.startBackgroundTransaction('open ai tx', async () => {
    const chatCompletion = await openai.chat.completions.create({
      max_tokens: 20,
      temperature: 0.5,
      messages: [{ role: 'user', content: process.env.MSG || 'Say this is a test'}],
      model: 'gpt-4',
    });
    newrelic.shutdown({ collectPendingData: true }, () => {
      console.log('done')
      process.exit(0)
    })
  })
}

main()

Related Issues

Closes #1841

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (20e7f1d) 96.85% compared to head (9218fef) 96.85%.

Files Patch % Lines
lib/instrumentation/openai.js 97.95% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #1862    +/-   ##
========================================
  Coverage   96.85%   96.85%            
========================================
  Files         206      208     +2     
  Lines       39252    39422   +170     
========================================
+ Hits        38017    38184   +167     
- Misses       1235     1238     +3     
Flag Coverage Δ
integration-tests-16.x 78.95% <66.66%> (+<0.01%) ⬆️
integration-tests-18.x 79.22% <66.66%> (+<0.01%) ⬆️
integration-tests-20.x 79.23% <66.66%> (+<0.01%) ⬆️
unit-tests-16.x 91.25% <56.86%> (-0.14%) ⬇️
unit-tests-18.x 91.23% <56.86%> (-0.14%) ⬇️
unit-tests-20.x 91.23% <56.86%> (-0.14%) ⬇️
versioned-tests-16.x 73.30% <92.81%> (+0.15%) ⬆️
versioned-tests-18.x 73.30% <92.81%> (+0.15%) ⬆️
versioned-tests-20.x 73.31% <92.81%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jsumners-nr
jsumners-nr previously approved these changes Nov 14, 2023
lib/instrumentation/openai.js Show resolved Hide resolved
lib/llm-events/openai/error-message.js Outdated Show resolved Hide resolved
test/unit/instrumentation/openai.test.js Outdated Show resolved Hide resolved
test/versioned/openai/openai.tap.js Show resolved Hide resolved
lib/shim/shim.js Show resolved Hide resolved
lib/shim/shim.js Show resolved Hide resolved
lib/shim/shim.js Show resolved Hide resolved
lib/instrumentation/openai.js Show resolved Hide resolved
lib/llm-events/openai/error-message.js Outdated Show resolved Hide resolved
test/unit/instrumentation/openai.test.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jsumners-nr jsumners-nr left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@bizob2828 bizob2828 merged commit 34dcd70 into newrelic:main Nov 15, 2023
46 checks passed
Node.js Engineering Board automation moved this from Needs PR Review to Done: Issues recently completed Nov 15, 2023
@github-actions github-actions bot mentioned this pull request Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Node.js Engineering Board
  
Done: Issues recently completed
Development

Successfully merging this pull request may close these issues.

Instrument chat creation - promise
2 participants