-
Notifications
You must be signed in to change notification settings - Fork 641
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
botocore: add basic tracing for bedrock ConverseStream #3204
base: main
Are you sure you want to change the base?
Conversation
...trumentation-botocore/src/opentelemetry/instrumentation/botocore/extensions/bedrock_utils.py
Outdated
Show resolved
Hide resolved
...etry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/extensions/types.py
Outdated
Show resolved
Hide resolved
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.
Looks good to me. Do you mind checking if this langchain code produces spans as intended?
from langchain_aws import ChatBedrock
def main():
llm = ChatBedrock(
beta_use_converse_api=True,
model_id='amazon.titan-text-lite-v1',
streaming=True,
)
response_stream = llm.stream("Write a short poem on OpenTelemetry.")
for chunk in response_stream:
for item in chunk.content:
if 'text' in item:
print(item['text'], end="")
if __name__ == "__main__":
main()
It does:
|
5241f21
to
9af5b68
Compare
9af5b68
to
bd8a50f
Compare
|
||
|
||
# pylint: disable=abstract-method | ||
class ConverseStreamWrapper(ObjectProxy): |
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.
what if stream terminates with an error? what if user closes it before receiving response fully? We should try to cover as much as we can and I believe OpenAI instrumentation does it
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'll cover more cases in followup PRs, thanks for reminding about this.
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.
Added in the checklist to not forgetting #3210
Description
This introduces basic tracing for the ConverseStream service. I've chosen to delay the closing of the span after the response has been consumed instead of buffering all the response chunks since the buffer it may become big.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.