-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[ENH]: add grpc python client interceptor #1802
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @nicolasgere and the rest of your teammates on Graphite |
Please tag your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]. See https://docs.trychroma.com/contributing#contributing-code-and-ideas |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
new_client_details = _ClientCallDetails( | ||
client_call_details.method, | ||
client_call_details.timeout, | ||
tuple(metadata), # Ensure metadata is a tuple |
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 chance metadata can contain things that cannot be properly serialized, e.g. nested or non-basic types?
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 don't expect anyone to add things to metadata.
# Log exception details and re-raise | ||
span.set_attribute("rpc.error", str(e)) | ||
span.set_status(StatusCode.ERROR, description=str(e)) | ||
raise |
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.
Would that not cause a failure of the GRPC message if we fail to process the trace? Should we differentiate between processing errors and GRPC invocation-related errors?
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.
otel should never raise, even if span is empty as far as I know
# 1. Instantiate the interceptor: interceptors = [OtelInterceptor()] | ||
# 2. Intercept the channel: channel = grpc.intercept_channel(channel, *interceptors) | ||
|
||
class OtelInterceptor( |
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.
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 it would be nice to have control over this, to help with integration with go/rust, distributed tracing, metrics etc
## Description of changes *Summarize the changes made by this PR.* - Improvements & Bug fixes - Add grpc client interceptor to propagate trace information for distributed tracing. ## Test plan *How are these changes tested?* Tested locally using tilt and golang rpc
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
Tested locally using tilt and golang rpc