OpenAI: instrument embeddings and chat completions #2398
Conversation
lib/new_relic/agent/instrumentation/net_http/instrumentation.rb
Outdated
Show resolved
Hide resolved
lib/new_relic/agent/instrumentation/ruby_openai/instrumentation.rb
Outdated
Show resolved
Hide resolved
Some attribute names are expected upstream to include periods. We can't use names with periods in setter/getter method names. To resolve this, a new ATTRIBUTE_NAME_EXCEPTIONS constant, which is accompanied by an attribute_name_exceptions method exists to store the string value of the attribute name. Then, replace_attr_with_string returns the correct string name when we're creating the final event_attributes hash.
Embedding and ChatCompletionSummary both need guids for their ids. ChatCompletionMessage does not. Let's set the guid as the default, and allow ChatCompletionMessage to override the guid.
Our instrumentation was changing the response when called in the begin block.
|
|
||
| executes do | ||
| if use_prepend? | ||
| if OPENAI_VERSION >= Gem::Version.new('5.0.0') |
There was a problem hiding this comment.
Cool to see the support for older versions.
| vendor: VENDOR, | ||
| conversation_id: conversation_id, | ||
| api_key_last_four_digits: parse_api_key, | ||
| request_max_tokens: parameters[:max_tokens] || parameters['max_tokens'], |
There was a problem hiding this comment.
What leads the hash to have symbols or strings for keys, and are there ever both types of key present in the same hash? It might be handy to have a helper method like this:
request_max_tokens: parameter_value(parameters, :max_tokens)
...
def parameter_value(parameters, value)
parameters[value] || parameters[value.to_s]
endand if we can rely on the hash keys being all symbols or all strings, we could further enhance the helper to memoize which key type is involved.
There was a problem hiding this comment.
Both Strings and Symbols are accepted as keys, so it's all up to the user on which they use. And yes—both types can be mix and matched in the same request.
Side note: We did performance testing of || vs kind_of? vs is_a? and found || to be the most performant by about 1.2x, which is why we went with an "or" check vs creating a helper method that would decide which to use.
| end | ||
|
|
||
| def parse_api_key | ||
| 'sk-' + headers['Authorization'][-4..-1] |
There was a problem hiding this comment.
Hmm... it looks like the ability to use [-4..] instead of [-4..-1] wasn't introduced until Ruby v2.6.
lib/new_relic/agent/instrumentation/ruby_openai/instrumentation.rb
Outdated
Show resolved
Hide resolved
| def attribute_name_exceptions | ||
| # TODO: OLD RUBIES < 2.6 | ||
| # Hash#merge accepts multiple arguments in 2.6, so we can reduce this | ||
| # to a single Hash#merge call with two arguments at that point |
There was a problem hiding this comment.
This is where better granular perf tests would help. If there's a significant performance gain to be had, it'd be worth writing the code both ways:
if RUBY_VERSION >= 2.6
new_way
else
old way
endThere was a problem hiding this comment.
Great call out! Ran a perf test a few times and item.merge(item1).merge(item2) is consistently ~1.25x slower. We will break this out into a conditional.
There was a problem hiding this comment.
Done! I wanted to use a ternary operator but it got super long 36d6cab
| sequence: index, | ||
| completion_id: summary_id, | ||
| vendor: VENDOR, | ||
| is_response: false |
There was a problem hiding this comment.
According to the spec, this shouldn't be set to false, it should be not included if it would be false
set to True if a message is the result of a chat completion and not an input message - omitted in False cases
There was a problem hiding this comment.
Great callout! Thank you!
There was a problem hiding this comment.
This has been updated! cdd42f6
merge(**args) is 1.25x more performant than merge.().merge(), so break out into a conditional untill Ruby <2.6 is dropped
lib/new_relic/agent/instrumentation/ruby_openai/instrumentation.rb
Outdated
Show resolved
Hide resolved
kaylareopelle
changed the title
Initial PR for OpenAI instrumentation. We're merging this into the
openai_instrumentationbranch to preserve one historical PR for our initial instrumentation rollout to hopefully make it easier for others to reference in the future.Co-authored by @hannahramadan
Closes #2403
Closes #2404
This PR includes:
ruby-openaigemaimultiverse runner groupai/ruby_openaitesting to the appropriate CI workflowsOpenAI::Client#embeddings:OpenAI::Client#chat, known in the spec as chat completions:LlmChatCompletionSummaryeventsLlmChatCompletionMessageeventsLlmEmbeddingandLlmChatCompletionSummaryevents from theNet::HTTPresponse headersllm: trueas an attribute on all transaction events with LLM-related segmentsIt also makes the following updates to the
Llm-namespaced classes/modules:LlmEventand otherLlmclasses to use the attribute key strings expected by the UIllm_eventattribute toAbstractSegmentThis PR does not contain the following GA-required tasks:
Net::HTTPresponse header assignment to the LLM eventsai_monitoring.enabledconfiguration optionThese items will be opened in separate PRs that we'll merge into the
openai_instrumentationfeature branch before GA.