-
Notifications
You must be signed in to change notification settings - Fork 580
feat(instrumentation-aws-sdk): add gen ai instrumentation for InvokeModel API #2777
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(instrumentation-aws-sdk): add gen ai instrumentation for InvokeModel API #2777
Conversation
…y-js-contrib into aws-bedrock-extension
…y-js-contrib into aws-bedrock-extension
config: AwsSdkInstrumentationConfig | ||
) { | ||
const currentModelId = response.request.commandInput?.modelId; | ||
if (response.data?.body) { |
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 there's no response body should this generate some debug log?
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 don't think we need to since an empty response body would not necessarily indicate broken client behavior, though I can't imagine an instance of this occurring.
plugins/node/opentelemetry-instrumentation-aws-sdk/test/bedrock-runtime.test.ts
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/bedrock-runtime.ts
Outdated
Show resolved
Hide resolved
d8adc04
to
be0b075
Compare
package-lock.json
Outdated
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.
Can you also undo the changes to package-lock.json
, since there are no changes to any package.json
here.
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.
should be fixed now.
16a18d3
to
751edd1
Compare
f898864
to
aeda8e6
Compare
Tagging @anuraaga since he is also working on features related to bedrock and might be interested, but I don't think your changes will conflict with his anyways. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2777 +/- ##
==========================================
+ Coverage 89.46% 89.60% +0.14%
==========================================
Files 174 174
Lines 8322 8450 +128
Branches 1592 1660 +68
==========================================
+ Hits 7445 7572 +127
- Misses 877 878 +1
🚀 New features to boost your workflow:
|
@ezhang6811 - looks like the lint step is failing - please also run |
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.
proxy-approving for @jj22ee (component owner)
Which problem is this PR solving?
Short description of the changes