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

Add session id to traces and a new export endpoint to export traces for a session #525

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dineshyv
Copy link
Contributor

What does this PR do?

  • There is some issue with the decorator pattern of span creation when using async generators, where there are multiple yields with in the same context. I think its much more explicit by using the explicit context manager pattern using with. I moved the span creations in agent instance to be using with
  • Inject session id at the turn level, which should quickly give us all traces across turns for a given session
  • Adds a new endpoint to export traces based on session id

Addresses #509

Test Plan

llama stack run /Users/dineshyv/.llama/distributions/llamastack-together/together-run.yaml
PYTHONPATH=. python -m examples.agents.rag_with_memory_bank localhost 5000
curl "http://localhost:5000/alpha/telemetry/get-traces-for-session" \
-X POST \
-H "Content-Type: application/json" \
-d '{
    "session_id": "62c974d9-da34-4e3c-9831-d81e233882c4",
    "lookback": "2h",
    "limit": 100
}'  | jq

Output of export: https://pastebin.com/Y2mxMQXR

Screenshot 2024-11-25 at 4 09 08 PM

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Nov 26, 2024

@webmethod(route="/telemetry/get-traces-for-session", method="POST")
async def get_traces_for_session(
self, session_id: str, lookback: str = "1h", limit: int = 100
Copy link
Contributor

@yanxi0830 yanxi0830 Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of questions:

  1. Should we make lookback window to be int in second or smt? What does the limit mean?

  2. Is the session_id specific to the Agent? Wondering how do we deal with llama-stack apps built with inference/chat_completion API call only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The argument for lookback is actually a string for jaeger as well. I think its easier to specify the window using 1h or 5s instead of int.
  2. If its not part of a session, we should be able to get the trace using just the trace id. We already have a endpoint for that, but not implemented for open telemetry. I can add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syned offline. We will be adding traces at the individial API level rather than focusing just on the agent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants