-
Notifications
You must be signed in to change notification settings - Fork 619
feat(instrumentation-aws-sdk): add Bedrock InvokeModelWithResponseStream instrumentation #2845
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 Bedrock InvokeModelWithResponseStream instrumentation #2845
Conversation
09f9777 to
eeb1e84
Compare
| case 'InvokeModel': | ||
| return this.requestPreSpanHookInvokeModel(request, config, diag); | ||
| case 'InvokeModelWithResponseStream': | ||
| return this.requestPreSpanHookInvokeModelWithResponseStream( |
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 there a reason to not re-use requestPreSpanHookInvokeModel: add a 4th isStream argument and pass in false for 'InvokeModel', true for 'InvokeModelWithResponseStream', and then make the minor update to the implementation? This is how it was done for 'Converse' and 'ConverseStream'.
It looks to me like the requestPreSpanHookInvokeModel and requestPreSpanHookInvokeModelWithResponseStream functions are almost identical ... except that the latter doesn't have blocks for 'meta.llama', 'cohere.*', and 'mistral'.
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 the suggestion, @trentm ! You are absolutely right! I've updated the code to consolidate requestPreSpanHookInvokeModel and requestPreSpanHookInvokeModelWithResponseStream into a single method using isStream parameter as you suggested.
eeb1e84 to
0d197f0
Compare
| ): Promise<any> { | ||
| const stream = response.data?.body; | ||
| const modelId = response.request.commandInput?.modelId; | ||
| if (!stream || !span.isRecording()) return; |
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 (!stream || !span.isRecording()) return; | |
| if (!stream) return; |
!span.isRecording() is already checked before responseHookInvokeModelWithResponseStream() is called
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.
You are right, @jj22ee! I removed this unnecessary check
| return response.data; | ||
|
|
||
| // Tap into the stream at the chunk level without modifying the chunk itself. | ||
| function instrumentAsyncIterable<T>( |
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.
Wouldn't it be more efficient to declare these following functions outside of this member function scope?
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.
Thank you, @jj22ee , for the review and good catches! Yes, these helper functions could be declared outside the method, at the class level, instead of being re-created every time the method is called.
Changed!
fe8416b to
ff1cb2d
Compare
|
Update this to pull in the main branch and I can take another look over. Thanks! |
| parsedChunk, | ||
| span | ||
| ); | ||
| } |
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.
The non-stream responseHookInvokeModel() is adding some attributes for a few more models: 'meta.llama', 'cohere.command-r', etc.
Should this function also add relevant attributes for those models? Or is it possible streaming is not supported for those models?
Also, perhaps the recordNovaAttributes(), recordClaudeAttributes(), etc. methods could be used by both responseHookInvokeModel and responseHookInvokeModelWithResponseStream. Or perhaps all the record*Attribute() methods could be moved to one setInvokeModelResponseAttributes() that is used by both responseHookInvokeModel*() methods.
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.
Done
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/bedrock-runtime.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/bedrock-runtime.ts
Outdated
Show resolved
Hide resolved
|
@yuliia-fryshko do you have bandwidth to work on this? Looks like there are some unaddressed comments that are blocking this from getting merged. |
0e2fef3 to
dcd6b28
Compare
Hi @pichlermarc ! Sorry for the long reply, I just back from the vacation today and I will try to address all comments today |
1941501 to
d86f003
Compare
71c7a6e to
f7bbec1
Compare
trentm
left a comment
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'm good with this PR now. Sorry, I realize this has dragged on for a long while.
Some notes:
- I have a nit about error handling (see below).
- At this point, because
gen_aisemconv moves quickly enough, some of the semconv vars here are outdated. E.g.gen_ai.systemis now replaced withgen_ai.provider.name. However, this instrumentation was already using the older gen-ai semconv version and the current gen-ai semconv docs (https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-agent-spans/) suggest that existing instrumentations should stick to their current version by default:
SHOULD NOT change the version of the GenAI conventions that they emit by default. Conventions include, but are not limited to, attributes, metric, span and event names, span kind and unit of measure.
SHOULD introduce an environment variable OTEL_SEMCONV_STABILITY_OPT_IN ...
So any updates of semconv version could be done in a separate PR.
| for await (const chunk of stream) { | ||
| const parsedChunk = this.parseChunk(chunk?.chunk?.bytes); | ||
|
|
||
| if (!parsedChunk) return; |
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.
nit: If, for whatever reason, a chunk cannot be parsed for adding otel data, should we perhaps continue to yield the chunks and eventually span.end() rather than return; 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.
I agree, it will be better to close the span in this case. Done
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 your change in commit 80d93e3 is not correct. It would result in span.end() being called and then an attempt to call BedrockRuntimeServiceExtension.record*Attributes(...) which would likely break.
I've change it to what I think it should be in commit ee6f51e.
@yuliia-fryshko Please let me know if my change looks inappropriate.
Thank you, @trentm ! I created an issue to review and upgrade all fields with latest semantic convention. Happy to take it after this MR will be merged. |
…emetry attributes, but yield the chunk as if there were no telemetry
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.
Just noticed that this file got added accidentally at some point.
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.
removed in commit 35143a4
|
Thank you for your contribution @yuliia-fryshko! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
Which problem is this PR solving?
Adds instrumentation of the InvokeModelWithResponseStreamCommand in the AWS Bedrock SDK.
Short description of the changes
instrumentAsyncIterableis used to inspect streamed chunks in real time and extract relevant telemetry.