-
Notifications
You must be signed in to change notification settings - Fork 596
feat(instrumentation-openai): add instrumentation of openai SDK #2941
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
Conversation
}) | ||
).rejects.toThrow(OpenAI.APIConnectionError); | ||
|
||
// TODO: Figure out why it takes so long to get this span. trace.getTracerProvider()._delegate.forceFlush() didn't help. |
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.
Could use some help debugging this
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2941 +/- ##
==========================================
- Coverage 89.86% 89.81% -0.06%
==========================================
Files 189 192 +3
Lines 9314 9638 +324
Branches 1911 1988 +77
==========================================
+ Hits 8370 8656 +286
- Misses 944 982 +38
🚀 New features to boost your workflow:
|
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 starting this, Anuraag. This is just a quick start at looking. I haven't looked at src/ or test/ yet.
body.content = msg.content; | ||
} | ||
} | ||
this.logger.emit({ |
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.
If one has set up a global logging provider, is it possible to opt out of the logs emitted from this instrumentation?
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.
@seemk I think one could do so by setting a NoopLoggerProvider on the instrumentation via instr.setLoggerProvider(noopLoggerProvider)
.
But are you asking for something different than that?
I haven't been following the GenAI semconv discussions of late. I vaguely recall that there was discussion of making it user-optional whether GenAI instrumentations used log-events or span attributes. Is that what you are referring to?
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.
Yeah - from what I understand, the conventions imply that when logs are enabled, logs are always emitted - this is how it currently works in the bedrock instrumentation of this repo, as well as genai in python and java. If it seems like there should be more knobs, we'd need to raise changes to the semconv but for now this implements the current practice.
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.
Another round of review, looking at the implementation this time. Looks good! Thanks for all the type improvements.
I haven't yet looked at the tests.
Co-authored-by: Trent Mick <[email protected]>
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.
Tests look good to me (with some comments below). I didn't comb through all 3000 lines; about half of them.
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 couple more things: test-all-versions, and openai@5 support.
This LGTM. Not sure if you were going to wait for feedback from the MS folks that might help and possibly be component owners. Before or when being taken out of draft, this PR will need to add one or more component owners to ".github/component_owners.yml". I could be one if needed, at least for starters. |
const EVENT_GEN_AI_USER_MESSAGE = 'gen_ai.user.message'; | ||
const EVENT_GEN_AI_ASSISTANT_MESSAGE = 'gen_ai.assistant.message'; | ||
const EVENT_GEN_AI_TOOL_MESSAGE = 'gen_ai.tool.message'; | ||
const EVENT_GEN_AI_CHOICE = 'gen_ai.choice'; |
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.
FWIW, I've started a PR to eventually add EVENT_*
exports from the semantic-conventions package.
open-telemetry/opentelemetry-js#5832
Thanks @trentm - I have gone ahead and updated owners as if possible, it would be nice to avoid drift by merging an initial version. Will continue discussion to hopefully add more /cc @paulshealy1 |
I asked for possible additional component owners for this instrumentation on the GenAI SIG today: https://docs.google.com/document/d/1EKIeDgBGXQPGehUigIRLwAUpRGa7-1kXB736EaYuJ2M/edit?tab=t.0#heading=h.tath31lg83xm Being a "component owner" means that you are listed for the particular instrumentation here: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/.github/component_owners.yml No pressure, but there was possible interest expressed by @zhirafovod. (@seemk would you be interested in being a component owner for this instrumentation?) As well, @singankit was possibly going to poke @paulshealy1 again internally. Thanks. |
I can help out, yeah |
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.
@anuraaga One suggested change (adding seemk as a component owner), then LGTM. Thanks!
Co-authored-by: Trent Mick <[email protected]>
Which problem is this PR solving?
Short description of the changes
Future PRs will add support for responses API and SDK 5
/cc @trentm if you can help review
/cc @lmolkova because I heard you are also interested in this instrumentation. Notably, I'd like this instrumentation to exist upstream so people can use it but need potential component owners I believe
This instrumentation is adapted from the one in https://github.com/elastic/elastic-otel-node/tree/main/packages/instrumentation-openai