feat: Add proxied tools functionality, specifically make sure it proxies tempo correctly #226
Conversation
|
Remaining test failures are (probably) because the tempo version that implements the mcp server isn't deployed to hosted grafana yet, so this may have to be left to sit for a while. |
tools/proxied_tools.go
Outdated
| // Also need Grafana config for the proxy handlers | ||
| grafanaURL := os.Getenv("GRAFANA_URL") | ||
| grafanaAPIKey := os.Getenv("GRAFANA_API_KEY") | ||
| if grafanaURL != "" { | ||
| gc := mcpgrafana.GrafanaConfig{ | ||
| URL: grafanaURL, | ||
| APIKey: grafanaAPIKey, | ||
| } | ||
| ctx = mcpgrafana.WithGrafanaConfig(ctx, gc) | ||
|
|
||
| // Create Grafana client | ||
| client := mcpgrafana.NewGrafanaClient(ctx, grafanaURL, grafanaAPIKey) | ||
| ctx = mcpgrafana.WithGrafanaClient(ctx, client) | ||
| } |
There was a problem hiding this comment.
hm this won't work in some cases where we don't set these settings using env vars, e.g. in the LLM app integration we extract them from context. in those cases we would need to do the proxying at call-time, since that's the only time we have access to a context with the URL/API key 😞
There was a problem hiding this comment.
I'm chewing on this, but I think I can probably do a small refactor to make it so this is importable into the llm app nicely
There was a problem hiding this comment.
I decoupled these variables from the proxied tools thing, so it should be possible to do this without worrying about that now. I poked around the plugin and i think we can actually get credentials on startup though, it looks like backend.GrafanaConfigFromContext works for this
SandersAaronD
force-pushed
the
sandersaarond/add-tempo
branch
from
aa31a24 to
6dbad36
Compare
|
Had a conversation with @sd2k about this, I think an extra concern to note is that when/if we have this set up to run as a hosted service that proxies to more than one grafana instance, this pattern is awkward. I don't think that should be blocking here, although it may mean that when we have this set up to work as a hosted service we should port the proxied tools up separately since there's a very different pattern surrounding them. |
SandersAaronD
force-pushed
the
sandersaarond/add-tempo
branch
from
58379dd to
188c6f7
Compare
SandersAaronD
force-pushed
the
sandersaarond/add-tempo
branch
from
2051453 to
58d0e7d
Compare
SandersAaronD
force-pushed
the
sandersaarond/add-tempo
branch
from
86e2cf6 to
58d0e7d
Compare
This seems good to my testing but it's hard to be sure; specifically the edge case around having two conflicting tempo MCP servers available from datasources may be buggy (since we only have one version on it currently!). Anyway, this sets up a flow for proxying tools via datasources and specifically carves out logic for handling tempo. Resolves #222