Conversation
Signed-off-by: Nabil-Salah <nabil.salah203@gmail.com>
Signed-off-by: Nabil-Salah <nabil.salah203@gmail.com>
dosubot
bot
added
the
feature vote
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
cmd/jaeger/internal/extension/jaegerquery/internal/jaegerai/handler.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Proof-of-concept wiring for an ACP-based “AI gateway” in Jaeger Query, with a local Python sidecar agent (Gemini-backed) reachable over WebSockets to demonstrate tool-calling (e.g., search_traces) end-to-end.
Changes:
- Adds a new
/api/chatHTTP endpoint in Jaeger Query that bridges ACP to a local WebSocket sidecar. - Introduces a Python ACP agent sidecar (Gemini client) plus minimal
uvproject scaffolding/docs to run it. - Adds Go dependencies for ACP and WebSockets; introduces a “dummy trace” shortcut in
FindTracesfor PoC tool output.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| python-sidecar/sidecar.py | Implements a WebSocket-bridged ACP agent that calls Gemini and can invoke search_traces. |
| python-sidecar/pyproject.toml | Declares Python-sidecar dependencies (agent-client-protocol, google-genai, websockets). |
| python-sidecar/main.py | Simple asyncio entrypoint wrapper for running the sidecar. |
| python-sidecar/README.md | Local run instructions and a curl-based manual test. |
| go.mod | Adds ACP Go SDK + gorilla/websocket dependencies. |
| go.sum | Adds checksums for the new Go dependencies. |
| cmd/jaeger/internal/extension/jaegerquery/querysvc/service.go | Adds a hardcoded dummy-service branch returning a synthetic trace. |
| cmd/jaeger/internal/extension/jaegerquery/internal/server.go | Registers the new /api/chat route (with basePath handling). |
| cmd/jaeger/internal/extension/jaegerquery/internal/jaegerai/handler.go | New HTTP handler implementing ACP client-side connection and tool plumbing. |
| Makefile | Excludes python-sidecar/ from repo script/lint/license checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if query.TraceQueryParams.ServiceName == "dummy-service" { | ||
| yield([]ptrace.Traces{dummyTrace(query.TraceQueryParams.OperationName)}, nil) | ||
| return | ||
| } |
| var req ChatRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| http.Error(w, "Bad request", http.StatusBadRequest) | ||
| return | ||
| } |
| c.w.Write([]byte(content.Text.Text)) | ||
| c.flusher.Flush() | ||
| } | ||
| } | ||
| if u.ToolCall != nil { | ||
| fmt.Fprintf(c.w, "\n[tool_call] %s\n", u.ToolCall.Title) | ||
| c.flusher.Flush() | ||
| } | ||
| if u.ToolCallUpdate != nil { | ||
| fmt.Fprintf(c.w, "\n[tool_result] id=%s status=%s\n", u.ToolCallUpdate.ToolCallId, valueOrUnknown(u.ToolCallUpdate.Status)) |
| if isinstance(message, str): | ||
| message = message.encode('utf-8') | ||
| client_writer.write(message) | ||
| if b'\n' not in message: |
| api_key = os.environ.get("GEMINI_API_KEY") | ||
|
|
||
| class JaegerSidecarAgent(Agent): | ||
| def __init__(self): | ||
| super().__init__() | ||
| self._conn: Client = None |
python-sidecar/sidecar.py
Outdated
| import asyncio | ||
| import json | ||
| import os | ||
| import socket | ||
| from urllib.parse import quote_plus |
| import asyncio | ||
|
|
||
| from sidecar import main as sidecar_main | ||
|
|
||
|
|
| dialer := websocket.Dialer{HandshakeTimeout: 5 * time.Second} | ||
| conn, resp, err := dialer.DialContext(ctx, "ws://localhost:9000", nil) | ||
| if resp != nil && resp.Body != nil { | ||
| defer resp.Body.Close() | ||
| } | ||
| if err != nil { | ||
| h.Logger.Error("Failed to dial ACP sidecar", zap.Error(err)) | ||
| http.Error(w, "Failed to connect to agent backend", http.StatusBadGateway) | ||
| return |
| if err != nil { | ||
| fmt.Fprintf(w, "Error initializing agent: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| sess, err := acpConn.NewSession(acpCtx, acp.NewSessionRequest{ | ||
| Cwd: "/", | ||
| McpServers: []acp.McpServer{}, | ||
| }) | ||
| if err != nil { | ||
| fmt.Fprintf(w, "Error creating session: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| // This is blocking until the agent finishes processing the prompt | ||
| _, err = acpConn.Prompt(acpCtx, acp.PromptRequest{ | ||
| SessionId: sess.SessionId, | ||
| Prompt: []acp.ContentBlock{acp.TextBlock(req.Prompt)}, | ||
| }) | ||
| if err != nil { | ||
| fmt.Fprintf(w, "Error starting prompt: %v\n", err) | ||
| return | ||
| } |
| -not -path './vendor/*' \ | ||
| -not -path './idl/*' \ | ||
| -not -path './jaeger-ui/*' \ | ||
| -not -path './python-sidecar/*' \ |
| query TraceQueryParams, | ||
| ) iter.Seq2[[]ptrace.Traces, error] { | ||
| return func(yield func([]ptrace.Traces, error) bool) { | ||
| if query.TraceQueryParams.ServiceName == "dummy-service" { |
There was a problem hiding this comment.
since you are running Jaeger for this test anyway, just refresh the UI a few times for internal traces to be reported under jaeger service. Then you won't need this hack just to test.
python-sidecar/sidecar.py
Outdated
| async def main(): | ||
| async with websockets.serve(handle_websocket, "localhost", 9000): | ||
| print("Jaeger ACP Sidecar listening on ws://localhost:9000") | ||
| await asyncio.Future() | ||
|
|
||
| if __name__ == "__main__": | ||
| asyncio.run(main()) |
There was a problem hiding this comment.
I suggest moving these to main.py
| return PromptResponse(stop_reason="end_turn") | ||
|
|
||
|
|
||
| async def ws_to_client_writer(websocket, client_writer): |
There was a problem hiding this comment.
please move ws / comms functions to a separate file
python-sidecar/sidecar.py
Outdated
| "Call this tool whenever trace/span lookup data is needed before answering." | ||
| ) | ||
|
|
||
| search_traces_tool = types.Tool( |
There was a problem hiding this comment.
since we have Jaeger MCP server, I wonder if the ACP SDK can just interrogate that server for available tools and invoke them instead of you manually re-implementing the plumbing/binding
There was a problem hiding this comment.
try this
from google.adk.tools import McpToolset
from google.adk.tools.mcp_tool import StreamableHTTPConnectionParams
# 1. Define the connection to Jaeger MCP server
mcp_config = StreamableHTTPConnectionParams(
url="https://127.0.0.1:16687/mcp",
)
# 2. Create the toolset (this handles the "automatic construction")
tools = McpToolset(connections=[mcp_config])then below instead of tools=[search_traces_tool] use tools: tools.get_tools(), and then use tools.call_tool(fc.name, fc.args) instead of manually switching via if name == "search_traces"
There was a problem hiding this comment.
You mean that for the discovery part, I use the MCP server? but for tool calling, I use the current Meta tools
because i thought you wanted the "Mediated" Approach (ACP + MCP).
The Jaeger Go Proxy will implement the Mediated Approach for all tool calls. The remote agent will never directly communicate with Jaeger's internal database or MCP server. but when need a tool it call using the jaeger acp client
There was a problem hiding this comment.
Not just for discovery but for tool execution too. Right now when the agent tells you it wants to invoke a tool like search traces you wrote a bunch of code to implement that search, but we already have mcp server doing that, so your proxy simply needs to invoke mcp and pass the result back to the agent.
|
|
||
| ## End-to-End Test | ||
|
|
||
| 1. Start Jaeger gateway (`jaeger-dev`) in another terminal. |
| } | ||
|
|
||
| // AI Gateway Endpoints | ||
| aiHandlerPath := "/api/chat" |
There was a problem hiding this comment.
| aiHandlerPath := "/api/chat" | |
| aiHandlerPath := "/api/ai/chat" |
| ) | ||
|
|
||
| // WsReadWriteCloser wraps a gorilla websocket to implement io.ReadWriteCloser | ||
| type WsReadWriteCloser struct { |
There was a problem hiding this comment.
please move ws/comms related structs to separate files
| queryService *querysvc.QueryService | ||
| } | ||
|
|
||
| func searchTracesToolResult(ctx context.Context, queryService *querysvc.QueryService, query string) string { |
There was a problem hiding this comment.
see my comment in sidecar.py - I don't see why we need any special handling for tools since we already have the MCP server.
| flusher: flusher, | ||
| queryService: h.QueryService, | ||
| } | ||
| acpConn := acp.NewClientSideConnection(clientImpl, adapter, adapter) |
There was a problem hiding this comment.
is this the connection to the sidecar? please add comments like this
| Meta: map[string]any{ | ||
| "tools": []map[string]string{ | ||
| { | ||
| "name": "search_traces", |
There was a problem hiding this comment.
not following why tool definition is repeated here.
|
|
||
| sess, err := acpConn.NewSession(acpCtx, acp.NewSessionRequest{ | ||
| Cwd: "/", | ||
| McpServers: []acp.McpServer{}, |
There was a problem hiding this comment.
architecturally what does having MCP servers here mean?
|
|
||
| import websockets | ||
|
|
||
| from google import genai |
There was a problem hiding this comment.
it seems these SDKs also exist in Go, maybe then we don't need to run a sidecar, just run it inside query service ?
Signed-off-by: Nabil-Salah <nabil.salah203@gmail.com>
Signed-off-by: Nabil-Salah <nabil.salah203@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds a proof-of-concept “AI gateway” to Jaeger Query that proxies chat prompts to an ACP sidecar over WebSocket, with the sidecar calling Jaeger MCP tools and using Gemini for responses.
Changes:
- Adds a new Jaeger Query HTTP endpoint (
/api/ai/chat) that streams agent output and tool lifecycle updates. - Introduces a Python ACP sidecar (WebSocket server) that bridges Gemini ↔ ACP ↔ Jaeger MCP tools.
- Adds Go module dependencies for ACP and WebSocket support, plus a dummy trace injection for PoC testing.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/jaeger/internal/extension/jaegerquery/querysvc/service.go |
Adds “dummy-service” short-circuit that returns a synthetic trace. |
cmd/jaeger/internal/extension/jaegerquery/internal/server.go |
Registers the new AI chat HTTP route in the Query server mux. |
cmd/jaeger/internal/extension/jaegerquery/internal/jaegerai/handler.go |
Implements /api/ai/chat handler that dials the sidecar via WebSocket and runs an ACP session. |
cmd/jaeger/internal/extension/jaegerquery/internal/jaegerai/streaming_client.go |
Implements ACP client callbacks to stream agent text/tool updates to the HTTP response. |
cmd/jaeger/internal/extension/jaegerquery/internal/jaegerai/ws_adapter.go |
Wraps gorilla/websocket as an io.ReadWriteCloser for ACP connection plumbing. |
python-sidecar/sidecar.py |
Implements ACP agent and MCP tool bridge using Gemini + Google ADK, plus WS↔ACP stream bridging. |
python-sidecar/main.py |
Starts the sidecar WebSocket server on localhost:9000. |
python-sidecar/pyproject.toml |
Declares sidecar project metadata and dependencies. |
python-sidecar/README.md |
Documents setup and manual curl testing for the PoC. |
go.mod |
Adds github.com/coder/acp-go-sdk and github.com/gorilla/websocket. |
go.sum |
Adds module sums for new Go dependencies. |
Makefile |
Excludes python-sidecar/ from script formatting/license checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| requires-python = ">=3.14" | ||
| dependencies = [ | ||
| "agent-client-protocol>=0.8.1", | ||
| "google-adk>=1.10.0", | ||
| "google-genai>=1.68.0", | ||
| "websockets>=16.0", |
There was a problem hiding this comment.
requires-python is set to ">=3.14", but the code shown only uses features available in older Python 3.x (e.g., 3.10/3.11). Unless 3.14 is required for a specific dependency/runtime feature, consider lowering this to reduce friction for contributors and runtime environments.
| var req ChatRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| http.Error(w, "Bad request", http.StatusBadRequest) | ||
| return | ||
| } |
There was a problem hiding this comment.
The request body is decoded without any size limit, so a large POST can cause excessive memory usage. Consider wrapping r.Body with http.MaxBytesReader (and optionally validating req.Prompt length / non-empty) before decoding.
| for tool in adk_tools: | ||
| declaration = tool._get_declaration() | ||
| if declaration is not None: | ||
| function_declarations.append(declaration) |
There was a problem hiding this comment.
This relies on a private/underscored API (tool._get_declaration()), which is prone to breaking on dependency upgrades. If google-adk exposes a public way to get function declarations, prefer that; otherwise isolate this behind a small compatibility layer with clear version pinning.
| func (h *ChatHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodPost { | ||
| http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
|
|
||
| var req ChatRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| http.Error(w, "Bad request", http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| flusher, ok := w.(http.Flusher) | ||
| if !ok { | ||
| http.Error(w, "Streaming unsupported", http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "text/plain; charset=utf-8") | ||
| w.Header().Set("Cache-Control", "no-cache") | ||
| w.Header().Set("Connection", "keep-alive") | ||
|
|
There was a problem hiding this comment.
This introduces a new public HTTP endpoint (/api/ai/chat) with streaming behavior and external sidecar dependency, but there are no corresponding tests. Consider adding at least a basic handler test (similar to existing http_handler_test.go patterns) that exercises request validation, sidecar dial failure handling, and streaming output on a mocked websocket/ACP connection.
| class JaegerSidecarAgent(Agent): | ||
| def __init__(self): | ||
| super().__init__() | ||
| self._conn: Client = None |
There was a problem hiding this comment.
The Gemini client is constructed even when GEMINI_API_KEY is unset (api_key=None), so failures will happen later in a less obvious place. Consider validating api_key at startup and returning a clear error to the ACP client (or failing fast) when the environment variable is missing.
| self._conn: Client = None | |
| self._conn: Client = None | |
| if not api_key: | |
| raise RuntimeError( | |
| "GEMINI_API_KEY environment variable is not set; cannot initialize Gemini client." | |
| ) |
| 3. Send a chat prompt: | ||
|
|
||
| ```bash | ||
| curl -N -X POST http://localhost:16686/api/chat \ |
There was a problem hiding this comment.
The README's curl example posts to /api/chat, but the server registers the handler at /api/ai/chat. Please update the example (or the route) so users can follow the documented steps successfully.
| curl -N -X POST http://localhost:16686/api/chat \ | |
| curl -N -X POST http://localhost:16686/api/ai/chat \ |
| You can start the same server using either entrypoint: | ||
|
|
||
| ```bash | ||
| uv run python sidecar.py | ||
| ``` | ||
|
|
||
| or | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
README suggests uv run python sidecar.py starts the server, but sidecar.py currently only defines handlers/classes and does not start a websocket server in a main block. Either add an entrypoint to sidecar.py or adjust the README to only reference main.py (or a proper console script).
| You can start the same server using either entrypoint: | |
| ```bash | |
| uv run python sidecar.py | |
| ``` | |
| or | |
| ```bash | |
| Start the sidecar server with: | |
| ```bash |
| -not -path './vendor/*' \ | ||
| -not -path './idl/*' \ | ||
| -not -path './jaeger-ui/*' \ | ||
| -not -path './python-sidecar/*' \ |
There was a problem hiding this comment.
Excluding python-sidecar from SCRIPTS_SRC means these new .py/.md files won't be covered by the repo's formatting/license checks (make fmt / make lint-license). If this code is intended to live in-repo (even as a PoC), consider keeping it in the lint set and adding the standard license headers instead of opting out.
| -not -path './python-sidecar/*' \ |
| if query.TraceQueryParams.ServiceName == "dummy-service" { | ||
| yield([]ptrace.Traces{dummyTrace(query.TraceQueryParams.OperationName)}, nil) | ||
| return | ||
| } |
There was a problem hiding this comment.
This injects a synthetic trace whenever the caller searches for service.name == "dummy-service", which changes production query behavior and could leak into real deployments (unexpected data, confusing UX, test-only behavior in main code path). Please gate this behind an explicit debug/POC configuration flag (or build tag), or move it to a dedicated mock storage/test harness instead of QueryService.FindTraces().
| dialer := websocket.Dialer{HandshakeTimeout: 5 * time.Second} | ||
| conn, resp, err := dialer.DialContext(ctx, "ws://localhost:9000", nil) |
There was a problem hiding this comment.
The websocket sidecar endpoint is hard-coded to ws://localhost:9000, which makes the feature unusable in non-local deployments and complicates ops (sidecar on different host/port, TLS, etc.). Please make the sidecar URL configurable (e.g., via QueryOptions / env var) and validate it at startup.
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
make lint testAI Usage in this PR (choose one)
See AI Usage Policy.