[ATO-1652] Add gRPC support #1109
Conversation
|
Still working on integration tests, unit tests and CI. |
rasa_sdk/grpc_server.py
Outdated
| initialise_interrupts(server) | ||
| executor = ActionExecutor() | ||
| executor.register_package(action_package_name) | ||
| # tracer_provider = get_tracer_provider(endpoints) |
There was a problem hiding this comment.
Need to add support for tracing.
@ancalita I might need your help on this one.
rasa_sdk/tracing/utils.py
Outdated
| ) -> Tuple[Any, Any, Text]: | ||
| """Gets tracer and context.""" | ||
| span_name = "create_app.webhook" | ||
| if tracer_provider is None: | ||
|
|
||
| if tracer_provider is None or isinstance(request, WebhookRequest): |
There was a problem hiding this comment.
Yeah we will need to update the code on rasa-private side too to inject the trace context in the grpc request. I am searching for some docs / looking in opentelemetry to see if there's any guidance.
There was a problem hiding this comment.
Couldn't find any guidance I'm afraid :(
There was a problem hiding this comment.
I'll try to look it up.
There was a problem hiding this comment.
@ancalita I added tracing for GRPCActionServerWebhook.Webhook.
radovanZRasa
force-pushed
the
feature/ATO-1652-add-grpc-support
branch
from
c2fe5a6
to
ed05393
Compare
radovanZRasa
force-pushed
the
feature/ATO-1652-add-grpc-support
branch
from
23461d5
to
b49f874
Compare
radovanZRasa
force-pushed
the
feature/ATO-1652-add-grpc-support
branch
from
a5ad38d
to
6cf9bdb
Compare
radovanZRasa
force-pushed
the
feature/ATO-1652-add-grpc-support
branch
from
fb8cd95
to
1e593ef
Compare
| self.tracer_provider = tracer_provider | ||
| self.executor = executor | ||
|
|
||
| async def Actions(self, request: ActionsRequest, context) -> ActionsResponse: |
There was a problem hiding this comment.
Same question here about not using the method args as above. Also context is missing type hint.
There was a problem hiding this comment.
I tried but I could not find the type of the context anywhere.
Considering method args, this is the cleanest way to do it according to protobuf docs.
It provides good options for backwards compatibility later on.
There was a problem hiding this comment.
Found the type of context after some digging through docs. It's grpc.aio.ServicerContext.
| @@ -17,10 +17,11 @@ def get_tracer_provider(endpoints_file: str) -> Optional[TracerProvider]: | |||
|
|
|||
|
|
|||
| def get_tracer_and_context( | |||
| tracer_provider: Optional[TracerProvider], request: Request | |||
| tracer_provider: Optional[TracerProvider], request: Union[Request] | |||
There was a problem hiding this comment.
This had the WebhookRequest before I think in the typing hint, could you please re-add it?
There was a problem hiding this comment.
No need. I create tracer in GRPCActionServerWebhook.Webhook.
There was a problem hiding this comment.
But that serves a different purpose, we need this for passing the rasa-private context to the sdk 🤔
There was a problem hiding this comment.
The tracer in the webhook instruments the webhook separately.
Add support to communicate with Rasa over gRPC protocol.
Status (please check what you already did):
black(please check Readme for instructions)