fix(grafana): validate against the API docs, add data source querying and contact-point CRUD - #6712
Merged
Merged
Conversation
The example rendered as tags="[''daily'']" — doubled single quotes from an escaping slip, which is not valid Kusto. The reference writes a tags list as tags='["TagA","TagB"]': single outer quotes with the JSON array's own double quotes inside. The clause builder already handled that form; only the example text was wrong. A template literal avoids the escaping entirely, since the metadata generator reads the source verbatim and would otherwise carry the backslashes into the description the model sees. Adds a test asserting the reference's exact multi-property clause round-trips, including the comma inside the quoted array.
…outbound request hardening Validated against Grafana's HTTP API reference and, where the docs contradict themselves, against the Go wire structs. Response shapes the tools got wrong: - update_annotation declared an `id` that was always 0; a patch returns only a message, so the request's annotation id is echoed and labelled as such - delete_folder discarded the numeric id Grafana returns and presented an input-echoed uid as if it came from the API - delete_dashboard fabricated `id: 0` / `title: ''` via `||` on absent fields - the contact-point `provenance` description was inverted: "api" means API-managed, empty means it stayed UI-editable Requests that could not succeed: - create_alert_rule left noDataState and execErrState unset and invisible to the model, but Grafana's validator rejects an empty value outright, so every model-driven create failed. Both are now sent with Grafana's own defaults, and skipped for recording rules, which take a different validator - get_data_source routed a numeric input at /api/datasources/:id, which exists only behind an off-by-default feature toggle. UID only now - list_annotations did not trim the dashboard UID, so a padded value matched nothing Outbound hardening on the three proxy routes: - the service-account token was re-sent to redirect targets; the shared fetch only drops it when asked, so stripAuthOnRedirect is now set - no timeout was passed, leaving two sequential hops at the 5-minute default - upstream error bodies were interpolated whole into the tool result, putting up to 10MB of HTML into logs and traces; now truncated - UID path segments are URL-encoded so they cannot re-target the request - update_folder sent both `version` and `overwrite: true`, which Grafana treats as alternatives, making the freshly fetched version decorative and silently clobbering a concurrent rename - replaced the `any` casts with narrowed types Block surface: - 25 outputs the tools emit were undeclared and so unreferenceable downstream; get_data_source had 13 of its 18 unreachable - `version` was typed string though the dashboard, folder, and data-source producers all emit a number - the dashboard title field was shown only for create, so a dashboard could never be renamed through Update Dashboard - six list outputs were typed json rather than array
…rafana-validation # Conflicts: # apps/sim/tools/generated/tool-metadata.ts # apps/sim/tools/generated/tool-outputs.ts
…e block outputs
The data source health check could only ever report health. Grafana answers an
unhealthy source with HTTP 400 carrying the same {status, message} payload as a
healthy one, and the tool framework converts any non-2xx into an opaque tool
error — so the diagnostic the caller actually wants was unreachable. The check
now goes through an internal route that reads the verdict off either status and
reports it as a successful check, while a failure carrying no verdict (missing
data source, bad token, plugin with no health endpoint) stays a real error. The
plugin's `details` payload is surfaced too.
Also on that route, matching the other three: an outbound timeout, redirect
auth stripping, a truncated upstream error, and a URL-encoded UID.
Block output descriptions: ten keys are emitted by several tools with different
meanings and were described for only one producer — `database` meant both a
data source name and a health status, `annotations` both an annotation list and
an alert rule's summary map. Eleven `json` outputs were opaque although the
tools already document their inner fields. All rewritten to name every producer.
Smaller alignment fixes:
- the same EmbeddedContactPoint.settings field was typed `object` in list and
`json` in create
- list_contact_points mapped non-nullable uid/name/type through `?? null`;
Grafana returns an empty string, which is what create already assumed
- create_alert_rule sent `orgID`, which Grafana overwrites from the
authenticated context, and `Number()` on a non-numeric value put NaN -> null
in the body
- the three update routes declared `output` as required though the auth
short-circuit omits it, and did not declare the `details` they emit on a
validation error
…ule-group read Four operations the integration was missing, taking it to 29. update_contact_point / delete_contact_point close a real gap: contact points could be listed and created but never corrected or removed. Two things worth recording, because the published docs get both wrong: - both verbs answer 202 with only a message, not the object. The rendered docs claim delete returns 204; the current spec and handler both say 202. So the UID is echoed from the request, the way delete_folder and update_annotation already do - update is a full replace with no PATCH counterpart, so name, type, and settings are all required and the description says so. Omitting disableResolveMessage resets it X-Disable-Provenance is exposed on update only. Its polarity is the opposite of the alert-rule case: omitting it always succeeds, while sending it against an API-provisioned contact point is rejected — with 403, not the 409 rules use. It is not exposed on delete at all, because that handler never reads stored provenance and the endpoint takes no such parameter. move_folder reuses get_folder's mapping verbatim — same DTO. It always sends the parentUid key, since Grafana reads an empty value as "move to the root", which a conditionally-omitted field could not express. get_alert_rule_group surfaces the group evaluation interval, the one alerting knob the per-rule operations cannot reach. It reuses the shared mapAlertRule for the nested rules, and the interval is documented as an integer of seconds.
…plates in real tools query_data_source closes the largest gap in the integration: 29 tools could read dashboards, folders, and alert configuration, but none could read a metric value. It posts to /api/ds/query and returns both the raw response and the frames flattened into rows. The flattening is derived from the documented layout rather than any data source's field names: a frame carries schema.fields[] alongside data.values[], where values[i] is the whole column for fields[i], so zipping them by position works for Prometheus, SQL, or anything else with a backend. A failed query is a 400 by Grafana's own status table, so it stays a tool error — unlike the health check, where the failure status carries the answer. That also lets four templates and the review-firing-alerts skill stop promising things the integration could not do. Three templates assumed a metric-query tool, which now exists. The fourth, and the skill, assumed live alert instance state, which the provisioning API never returns — they now derive firing rules from alert-state annotations, which are documented to carry newState and prevState, and say so explicitly rather than implying a live snapshot. Deliberately not added: a tool over /api/prometheus/grafana/api/v1/rules for live instance state. That endpoint appears on no Grafana HTTP API doc page, its response is only readable from Go internals and test assertions, and the instance-level state casing differs from the rule level with no documented contract. Not something to build an output schema on.
…rafana-validation
Renaming update_annotation's phantom `id` to `annotationId` and adding `details` to the health check both created outputs the block never declared, so neither was referenceable downstream. Caught by re-running the output-coverage check over both integrations; the block now covers all 64 keys the 30 tools emit.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview A dedicated data source health route treats Grafana’s HTTP 400 unhealthy payloads as successful tool output instead of opaque errors. Outbound proxy routes add timeouts, New tools: Azure Data Explorer docs fix the Reviewed by Cursor Bugbot for commit 02fb1f6. Configure here. |
Contributor
Greptile SummaryThe PR expands the Grafana integration with data-source querying, contact-point CRUD, folder movement, and alert-rule-group retrieval while correcting API request, response, proxy, and documentation behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/blocks/blocks/grafana.ts | Adds UI fields, operation selection, parameter mapping, and declared outputs for the new and corrected Grafana tools; the prior contact-point update omission is fixed. |
| apps/sim/lib/api/contracts/tools/grafana.ts | Expands Grafana API response contracts and documents why provider-specific nested fields remain unconstrained; the previous rationale concern is resolved. |
| apps/sim/tools/grafana/query_data_source.ts | Adds generic Grafana data-source querying and converts columnar response frames into referenceable rows. |
| apps/sim/app/api/tools/grafana/check_data_source_health/route.ts | Proxies health checks so Grafana’s structured unhealthy verdict remains available even when upstream returns a non-success status. |
| apps/sim/tools/grafana/update_contact_point.ts | Adds full-replacement contact-point updates with required UID, name, type, and settings inputs. |
Reviews (2): Last reviewed commit: "fix(grafana): make Update Contact Point ..." | Re-trigger Greptile
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The new replace operation could never succeed. contactPointType and contactPointSettings were widened to cover it, but contactPointNameNew was left create-only — and the update maps `name` from that field, so the required parameter was never supplied. disableResolveMessage had the same gap, and it matters more than it looks: the update is a full replace, so a block-driven update was silently clearing resolve suppression on every contact point it touched. Both fields are now shown, and required where the API requires them. Also states a reason on each intentionally-unconstrained response field — Zod issue objects, alert query stages, notification settings, recording-rule config, and data-source health detail are all genuinely opaque, but that was left implicit.
Collaborator
Author
Collaborator
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 02fb1f6. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tagsingestion-property example, which rendered as invalid Kusto (tags="[''daily'']"); the reference writes a tags list astags='["TagA","TagB"]'Grafana: response shapes the tools got wrong
update_annotationdeclared anidthat was always0— a patch returns only a message, so the request's annotation id is echoed and labelled as suchdelete_folderdiscarded the numeric id Grafana returns and presented an input-echoed uid as an API resultdelete_dashboardfabricatedid: 0/title: ''via||on absent fieldsprovenancedescription was inverted:"api"means API-managed, empty means it stayed UI-editableget_data_sourcehad 13 of its 18 unreachableversionwas typed string though the dashboard, folder, and data-source producers all emit a numberjsonoutputs were opaque although the tools document their inner fieldsGrafana: requests that could not succeed
create_alert_ruleleftnoDataStateandexecErrStateunset and invisible to the model, but Grafana's validator rejects an empty value outright, so every model-driven create failed. Both are now sent with Grafana's own defaults, and skipped for recording rules, which take a different validatorget_data_sourcerouted numeric input at/api/datasources/:id, which exists only behind an off-by-default feature togglelist_annotationsdid not trim the dashboard UID, so a padded value matched nothingGrafana: the health check could only report health
Grafana answers an unhealthy data source with HTTP 400 carrying the same
{status, message}payload as a healthy one, and the tool framework converts any non-2xx into an opaque error — so the diagnostic was unreachable. It now goes through an internal route that reads the verdict off either status, while a failure carrying no verdict stays a real error.Grafana: outbound hardening on the proxy routes
update_foldersent bothversionandoverwrite: true, which Grafana treats as alternatives, making the freshly fetched version decorative and silently clobbering a concurrent renameanycasts with narrowed typesNew Grafana operations
query_data_source— the largest gap: 29 tools could read configuration but none could read a metric value. Returns the raw/api/ds/queryresponse plus frames flattened into rows, derived from the documentedschema.fields[]/data.values[]columnar layout so it works for any backend data sourceupdate_contact_point/delete_contact_point— contact points could be listed and created but never corrected or removed. Both verbs answer 202 with only a message, so the UID is echoed; update is a full replace with no PATCH counterpartmove_folder— reusesget_folder's mapping; always sendsparentUidsince Grafana reads an empty value as "move to root"get_alert_rule_group— surfaces the group evaluation interval, the one alerting knob the per-rule operations cannot reachFour templates and the
review-firing-alertsskill previously promised metric queries and live alert state. Three are now grounded inquery_data_source; the fourth and the skill derive firing rules from alert-state annotations, which are documented to carrynewState/prevState, and say so rather than implying a live snapshot.Deliberately not added: a tool over
/api/prometheus/grafana/api/v1/rulesfor live instance state. That endpoint appears on no Grafana HTTP API doc page and its response is only readable from Go internals, so there is no contract to build an output schema on.Type of Change
Testing
813 tests pass, including new coverage for the health-check route and the ADX clause builder; every new guard verified to fail when removed. Type-check clean, all 26 audits pass, docs regenerated.
Checklist