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 openai LLM events. #1857

Merged
merged 3 commits into from Nov 13, 2023
Merged

Conversation

bizob2828
Copy link
Member

@bizob2828 bizob2828 commented Nov 9, 2023

Description

I was able to confirm the data exists on the given request/responses when using a real openai library. I also removed api_type as it is not available. also updated CDD for how to get organization

Links

Closes #1843

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Merging #1857 (3422336) into main (3b1f5e6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 3422336 differs from pull request most recent head 1cc98f8. Consider uploading reports for the commit 1cc98f8 to get more accurate results

@@            Coverage Diff             @@
##             main    #1857      +/-   ##
==========================================
+ Coverage   96.83%   96.85%   +0.01%     
==========================================
  Files         200      206       +6     
  Lines       39074    39236     +162     
==========================================
+ Hits        37839    38001     +162     
  Misses       1235     1235              
Flag Coverage Δ
integration-tests-16.x 78.94% <ø> (ø)
integration-tests-18.x 79.22% <ø> (ø)
integration-tests-20.x 79.23% <ø> (ø)
unit-tests-16.x 91.38% <100.00%> (+0.03%) ⬆️
unit-tests-18.x 91.36% <100.00%> (+0.03%) ⬆️
unit-tests-20.x 91.36% <100.00%> (+0.03%) ⬆️
versioned-tests-16.x 73.15% <ø> (-0.01%) ⬇️
versioned-tests-18.x 73.15% <ø> (-0.01%) ⬇️
versioned-tests-20.x 73.16% <ø> (-0.01%) ⬇️

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

Files Coverage Δ
lib/llm-events/openai/chat-completion-message.js 100.00% <100.00%> (ø)
lib/llm-events/openai/chat-completion-summary.js 100.00% <100.00%> (ø)
lib/llm-events/openai/embedding.js 100.00% <100.00%> (ø)
lib/llm-events/openai/error-message.js 100.00% <100.00%> (ø)
lib/llm-events/openai/event.js 100.00% <100.00%> (ø)
lib/llm-events/openai/feedback-message.js 100.00% <100.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

jsumners-nr
jsumners-nr previously approved these changes Nov 9, 2023
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.

lib/llm-events/openai/error-message.js Outdated Show resolved Hide resolved
Comment on lines +16 to +19
let agent
t.beforeEach(() => {
agent = helper.loadMockedAgent()
})
Copy link
Contributor

Choose a reason for hiding this comment

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

As you mentioned in our call, defining agent this way technically makes it non-deterministic as to which instance is used by the subtests. But they run sequentially, so it's fine. But using t.context would assure the re-used variable is unique per subtest.

Not a blocker for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

yea this is a pattern all throughout our code. I guess it's a balance between consistency and correctness. Maybe we file some tech debt to remove saving references and relying on subtest context for passing data like this

jsumners-nr
jsumners-nr previously approved these changes Nov 13, 2023
@bizob2828 bizob2828 merged commit a7786f3 into newrelic:main Nov 13, 2023
22 checks passed
Node.js Engineering Board automation moved this from Needs PR Review to Done: Issues recently completed Nov 13, 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.

Create LLM event types
2 participants