-
Notifications
You must be signed in to change notification settings - Fork 600
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
[WIP] Create LLM Events #2414
[WIP] Create LLM Events #2414
Conversation
* make attrs optional * refactor out response headers * update class -> module for NewRelic::Agent * uncomment requires * remove attr_accessors
@id = id | ||
@request_id = request_id | ||
@span_id = span_id | ||
@transaction_id = transaction_id |
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.
for the things like this, we could probably have them automatically set to the current_transaction/span so we don't have to set it ourselves unless we want it different for some reason
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.
💯
thanks for catching this. meant to pull that out of the assignment and forgot! trace_id can probably be accessed that way too.
module Agent | ||
class LlmEvent | ||
class ChatCompletion < LlmEvent | ||
# TODO: should any of the attrs be required on initialization? |
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.
since there are so many attributes, i'm down for not having any be required immediately, and just letting things be settable, so we could create the object first and then add attributes to it as we go? Maybe adding a []= method or something would be nice so we could just be like event[:conversation_id] = 2
, or something like that? I think something like that would make the creation of all the attributes a little nicer to do than just having to provide everything in the initializer.
idk what does everyone think about something like that?
module NewRelic | ||
module Agent | ||
class LlmEvent | ||
# response_model looks like repsonse.model |
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.
# response_model looks like repsonse.model | |
# response_model looks like response.model |
# response_model looks like repsonse.model | ||
INGEST_SOURCE = 'Ruby' | ||
|
||
def initialize(id: nil, request_id: nil, span_id: nil, transaction_id: nil, trace_id: nil, response_model: nil, vendor: nil, **args) |
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.
Is the double splat intended to catch something in the future? If not, I'm thinking it can be removed or perhaps the method input can be changed to simply be a hash instead of named arguments.
module NewRelic | ||
module Agent | ||
class LlmEvent | ||
class ChatCompletion < LlmEvent |
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.
Is ChatCompletion
meant to be nested inside the LlmEvent
class AND inherit from it as well?
module Agent | ||
class LlmEvent | ||
class ChatCompletion | ||
class Message < ChatCompletion |
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 think I prefer or perhaps am simply accustomed to module namespacing instead of class namespacing. What does the class class class
give us here?
I think if this use case is foreseen that we should make use of
Perhaps each class could declare its attributes in an array. Then things like |
class LlmEvent | ||
# response_model looks like repsonse.model | ||
INGEST_SOURCE = 'Ruby' | ||
|
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.
does **args work below 2.7
Closing in favor of #2415 |
We need to create events with specific attributes regardless of what library is instrumented.
These classes are one strategy to solve this problem. There might be a better way to do this.
To reduce duplication, some attributes are passed down from parents/grandparents to child classes, ex. LlmEvents::ChatCompletion::Message has some of its attributes defined in LlmEvents, ChatCompletion and Messages. They all get merged in an attributes method in the child class, which in this example would be messages.
The child classes have their own record methods. These methods are intended to create the custom events. The record method is defined in parent/grandparent classes, but it is empty. Following patterns elsewhere in the agent, we don't raise an error if a child class hasn't defined the record method.
One way to use these classes:
Closes #2408
co-authored-by: @hannahramadan