-
Notifications
You must be signed in to change notification settings - Fork 101
Add loki tools #44
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 loki tools #44
Conversation
tools/loki.go
Outdated
// QueryLokiLogsParams defines the parameters for querying Loki logs | ||
type QueryLokiLogsParams struct { | ||
DatasourceUID string `json:"datasourceUid" jsonschema:"required,description=The UID of the datasource to query"` | ||
LogQL string `json:"logql" jsonschema:"required,description=The LogQL query to execute"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How well do you find the various LLMs at being able to generate LogQL? I ran into issues with valid LogQL at one point and went down a slightly different path where I instead prompted for label matchers and a string to match against, then constructed valid LogQL from that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it might be nice to have a few ways to provide a bit more context. I can think of a couple of ways:
- a tool which just provides a LogQL primer, so the model can use it before it needs to write LogQL. We could include some brief instructions in the query logs tool to say 'make sure to call the LogQL primer tool first'. That way the context isn't automatically expanded just by having the tool included, only when the model wants to use it.
- a resource which does the same thing, but that the user or client can choose to include (e.g. by inputting
@logql-primer
or something - however the client chooses to handle that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keen to try this but I would probably do it on a separate PR. After changing the descriptions the stats endpoint is called with a matcher so it seems improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks nice! A couple of suggestions, we might need to play around with the descriptions to they get decent results but this is a great start, thanks.
tools/loki.go
Outdated
// QueryLokiStatsParams defines the parameters for querying Loki stats | ||
type QueryLokiStatsParams struct { | ||
DatasourceUID string `json:"datasourceUid" jsonschema:"required,description=The UID of the datasource to query"` | ||
LogQL string `json:"logql" jsonschema:"required,description=The LogQL query to execute"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth noting in the description that this can't be arbitrary LogQL, but only matchers (see https://grafana.com/docs/loki/latest/reference/loki-http-api/#query-log-statistics).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the descriptions a bit. Trying to test this but again ml instance is down :(
These Anthropic docs about good tool descriptions might be helpful! (we could do with improving lots of others, too) |
Co-authored-by: Ben Sully <[email protected]>
That's really useful @sd2k thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the README too? I think that's all that's left!
tools/loki.go
Outdated
// QueryLokiLogs is a tool for querying logs from Loki | ||
var QueryLokiLogs = mcpgrafana.MustTool( | ||
"query_loki_logs", | ||
"Query and retrieve log entries from a Loki datasource using LogQL. Returns log lines with their timestamps and associated label metadata. Supports full LogQL syntax including filters and expressions.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding something like "Before using this tool, use query_loki_stats
to ensure the log stream is not too large, list_loki_label_names
to ensure the labels exist, and list_loki_label_values
to ensure the label values exist.
2dd762c
to
3cecb74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor nits but lgtm!
Co-authored-by: Ben Sully <[email protected]>
Co-authored-by: Ben Sully <[email protected]>
Co-authored-by: Ben Sully <[email protected]>
Related to this issue
In this PR we are adding tools to:
from Loki datasources.
TODO: