Skip to content
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

Add support for ConverseStream to bedrock instrumentation #13410

Merged
merged 9 commits into from
Mar 12, 2025

Conversation

anuraaga
Copy link
Contributor

Unfortunately needed to add a client wrapper for library instrumentation users to be able to intercept the response stream, otherwise main difference from non-streaming instrumentation is to propagate that interceptor's data through Context.

/cc @codefromthecrypt

@anuraaga anuraaga requested a review from a team as a code owner February 27, 2025 06:19
* BedrockRuntimeAsyncClient} instance.
*/
@NoMuzzle
public BedrockRuntimeAsyncClient wrapBedrockRuntimeClient(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't use wrap as was probably intended because all argument classes for overloads have to be on the compile classpath, which isn't the case. If the library instrumentation hasn't been marked stable yet, in a separate PR I could deprecate wrap and add wrapSqsClient if it makes sense

Copy link

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds sensible.

Also for those not familiar the reason these bodies are base64 is due to Amazon's stream format which is binary

String stopReason = BedrockRuntimeAccess.getStopReason(response.getSdkResponse());
if (stopReason == null) {
List<String> stopReasons = BedrockRuntimeAccess.getStopReasons(response);
if (stopReasons == null) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we currently return empty list instead of null from other getters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I think we can't do that for the request stop sequences to differentiate from user setting to empty vs not setting, but for response here empty seems good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also remove the @Nullable. When returning empty instead of null you may also need to change the extractor as internalSet only skips null attributes, but not the empty ones. Idk if that matters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, removed @Nullable and had the extractor check for empty. I left the Getter permitting null anyways since I couldn't see a strong reason to be too strict on it (I noticed MessagingAttributesGetter.getMesssageHeader being strict though)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave this to @trask to decide whether it makes sense to keep the @Nullable on the getter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with @nullable List as long as there's a comment in the code explaining what the difference between null and empty signals to the caller since it's never obvious to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - actually no comment comes to mind so I removed the Nullable but kept the null check to avoid crashing if instrumentation has a bug, realized that's probably closer to the practice of what's in the OTel APIs.

@trask
Copy link
Member

trask commented Mar 10, 2025

thanks @anuraaga! will merge once conflicts are resolved

@anuraaga
Copy link
Contributor Author

Thanks, fixed the conflicts

@anuraaga
Copy link
Contributor Author

Merged main for the muzzle failure

@trask trask merged commit 2f7b59f into open-telemetry:main Mar 12, 2025
85 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants