-
Notifications
You must be signed in to change notification settings - Fork 199
Added matchers in listLokiLabelValues #380
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
base: main
Are you sure you want to change the base?
Added matchers in listLokiLabelValues #380
Conversation
|
|
tools/loki.go
Outdated
| LabelName string `json:"labelName" jsonschema:"required,description=The name of the label to retrieve values for (e.g. 'app'\\, 'env'\\, 'pod')"` | ||
| StartRFC3339 string `json:"startRfc3339,omitempty" jsonschema:"description=Optionally\\, the start time of the query in RFC3339 format (defaults to 1 hour ago)"` | ||
| EndRFC3339 string `json:"endRfc3339,omitempty" jsonschema:"description=Optionally\\, the end time of the query in RFC3339 format (defaults to now)"` | ||
| Matchers string `json:"matchers"` |
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.
Hi @rahulvijil,
thanks for your contribution. Would you mind adding two things to this matchers prop:
omitempty- A description that would help the LLM understand what this parameter is used for.
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.
Added the description
| return nil, fmt.Errorf("creating Loki client: %w", err) | ||
| } | ||
|
|
||
| // Use the client's fetchData method | ||
| urlPath := fmt.Sprintf("/loki/api/v1/label/%s/values", args.LabelName) | ||
|
|
||
| // Manually include matchers in query string if provided | ||
| if args.Matchers != "" { | ||
| encoded := url.QueryEscape(args.Matchers) | ||
| urlPath = fmt.Sprintf("%s?query=%s", urlPath, encoded) | ||
| } | ||
|
|
||
| // Call the original fetchData with start/end as before | ||
| result, err := client.fetchData(ctx, urlPath, args.StartRFC3339, args.EndRFC3339) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf("fetching label values: %w", err) | ||
| } | ||
|
|
||
| if len(result) == 0 { |
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.
Are these changes needed?
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.
Not actually, I will revert fmt.Errorf("fetching label values: %w", err) to err
tools/loki.go
Outdated
| func (c *Client) fetchData(ctx context.Context, urlPath string, startRFC3339, endRFC3339 string, optionals ...string) ([]string, error) { | ||
| params := url.Values{} | ||
| var optional string | ||
| if len(optionals) > 0 { | ||
| optional = optional[0] | ||
| } | ||
| if startRFC3339 != "" { | ||
| params.Add("start", startRFC3339) | ||
| } | ||
| if endRFC3339 != "" { | ||
| params.Add("end", endRFC3339) | ||
| } | ||
| if optional != "" { | ||
| params.Add("query", optional) | ||
| } | ||
| fullURL := fmt.Sprintf("%s?%s", urlPath, params.Encode()) | ||
| fmt.Println("Final request URL:", fullURL) |
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.
Hm, I think instead adding this here as optional, we might be better creating a new fetchLabels method.
sd2k
left a comment
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.
Noticed a couple of unnecessary printlns, otherwise this looks good.
| } | ||
| func (c *Client) handleAPICall(ctx context.Context, params url.Values, urlPath string) ([]string, error) { | ||
| fullURL := fmt.Sprintf("%s?%s", urlPath, params.Encode()) | ||
| fmt.Println("Final request URL:", fullURL) |
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 remove this Println?
| } | ||
|
|
||
| pretty, _ := json.MarshalIndent(labelResponse, "", " ") | ||
| fmt.Println("Loki Full Response:\n", string(pretty)) |
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.
This Println also needs removing
sd2k
left a comment
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.
Actually would you mind adding some integration tests please? You can follow the existing patterns in 'loki_test.go'.
🧩 Summary
This PR adds support for optional
matchersin thelist_loki_label_values()function to allow filtered label value queries in Loki.Previously, the MCP API did not accept
matchers, causing the function to always return unfiltered results (all label values across namespaces and apps).With this change, users can now pass Loki stream selectors (e.g.,
{app="...", namespace="..."}) to filter results as needed.🔍 Why this change was needed
My agent was calling the Grafana MCP API as follows:
Since the backend did not support the
matchersfield, the query ignored these filters and returned all values.This PR fixes that gap by forwarding the
matchersparameter as a query argument to Loki’s/loki/api/v1/label/{name}/valuesendpoint.Key Changes
Notes