-
Notifications
You must be signed in to change notification settings - Fork 1.4k
use metadata from SessionMessage to propagate related_request_id #591
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
Conversation
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.
LG, one question inline
session_message.metadata is not None | ||
and isinstance( | ||
session_message.metadata, | ||
ServerMessageMetadata, |
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.
Might they be passing their own object (say with added fields) that still has a related_request_id
? Not sure the SDK as currently implemented would be capable of handling that (without hacking anyway), but flagging in case we want to relax this to a hasattr
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.
that's actually the reason why typed metadata was introduced, before I was using meta field in the message, which was quite bad
…essage-for-related-request
…essage-for-related-request
The base branch was changed.
Refactoring the way we handle related request IDs in the streamable HTTP transport. Previously, we were adding _meta fields to notification parameters to link notifications to their triggering requests. This approach was fragile and inconsistent
with how other transports handle message metadata.
Changes
metadata
metadata
Note: stacked on top of #590
Improves related request id handling in #553