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

[ATO-2609] Add gRPC tracing #1117

Merged
merged 2 commits into from
Jul 2, 2024
Merged

Conversation

radovanZRasa
Copy link
Contributor

Add tracing between Rasa and action server.

image

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation in the rasaHQ/rasa
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@radovanZRasa radovanZRasa force-pushed the improvement/ATO-2609-add-grpc-tracing branch 2 times, most recently from b41e9ad to 9a135b4 Compare July 2, 2024 10:15
@radovanZRasa radovanZRasa requested a review from Tawakalt July 2, 2024 10:48
@radovanZRasa radovanZRasa force-pushed the improvement/ATO-2609-add-grpc-tracing branch from 9a135b4 to 308eadd Compare July 2, 2024 11:49
@Tawakalt
Copy link
Contributor

Tawakalt commented Jul 2, 2024

@radovanZRasa We should do a quick check that this works with grpc as well.

@ancalita
Copy link
Member

ancalita commented Jul 2, 2024

@radovanZRasa Do we know what are the errors in the screenshot you shared in the PR description? Referring to the spans with exclamation mark next to them.

@radovanZRasa
Copy link
Contributor Author

@radovanZRasa Do we know what are the errors in the screenshot you shared in the PR description? Referring to the spans with exclamation mark next to them.

Yea, I was testing Rasa with CALM bot from calm-demo-bot and action server from simple NLU bot because I did not want to lose time to install necessary dependencies for CALM bot's actions in my Rasa SDK virtual env. I only wanted to verify that tracing works.

@Tawakalt
Copy link
Contributor

Tawakalt commented Jul 2, 2024

@radovanZRasa Do we know what are the errors in the screenshot you shared in the PR description? Referring to the spans with exclamation mark next to them.

It's most likely the missing domain error log that should now be fixed.

@radovanZRasa
Copy link
Contributor Author

radovanZRasa commented Jul 2, 2024

@radovanZRasa Do we know what are the errors in the screenshot you shared in the PR description? Referring to the spans with exclamation mark next to them.

It's most likely the missing domain error log that should now be fixed.

They will also appear in this case. Good thing is that all calls are logged so everything is there.

image

As image shows all calls are recorded.

For failed call, log indicates that domain was not found:
image

@radovanZRasa
Copy link
Contributor Author

radovanZRasa commented Jul 2, 2024

@radovanZRasa We should do a quick check that this works with grpc as well.

I added custom action AddContact and added tracing into it:

from typing import Any, Dict
from rasa_sdk import Action, Tracker
from rasa_sdk.events import SlotSet
from rasa_sdk.executor import CollectingDispatcher
from rasa_sdk.tracing.tracer_register import ActionExecutorTracerRegister

class AddContact(Action):
    def name(self) -> str:
        return "add_contact"

    def run(
        self, dispatcher: CollectingDispatcher, tracker: Tracker, domain: Dict[str, Any]
    ):
        tracer = ActionExecutorTracerRegister().get_tracer()
        with tracer.start_as_current_span("AddContact.add_contact"):
            return [SlotSet("return_value", "success")]

Verified:
image

@radovanZRasa radovanZRasa force-pushed the improvement/ATO-2609-add-grpc-tracing branch from c8c92fb to 03dd122 Compare July 2, 2024 14:15
@radovanZRasa radovanZRasa merged commit 02840a3 into main Jul 2, 2024
16 checks passed
@radovanZRasa radovanZRasa deleted the improvement/ATO-2609-add-grpc-tracing branch July 2, 2024 14:19
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.

3 participants