Add support for ConverseStream to bedrock instrumentation #13410
Conversation
| * BedrockRuntimeAsyncClient} instance. | ||
| */ | ||
| @NoMuzzle | ||
| public BedrockRuntimeAsyncClient wrapBedrockRuntimeClient( |
There was a problem hiding this comment.
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
| if (stopReason == null) { | ||
| List<String> stopReasons = BedrockRuntimeAccess.getStopReasons(response); | ||
| if (stopReasons == null) { | ||
| return null; |
There was a problem hiding this comment.
I think that we currently return empty list instead of null from other getters
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I'll leave this to @trask to decide whether it makes sense to keep the @Nullable on the getter.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
anuraaga
force-pushed
the
aws-sdk-bedrock
branch
from
bc77bf2 to
6c57d34
Compare
|
thanks @anuraaga! will merge once conflicts are resolved |
…nstrumentation into aws-sdk-bedrock
|
Thanks, fixed the conflicts |
…nstrumentation into aws-sdk-bedrock
|
Merged main for the muzzle failure |
5 participants
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