[BUGFIX] Loki query migration incorrectly matching non-Loki datasources#682
[BUGFIX] Loki query migration incorrectly matching non-Loki datasources#682gfierkasap wants to merge 6 commits into
Conversation
Signed-off-by: gfierksap <gabor.fierka@sap.com>
Signed-off-by: gfierksap <gabor.fierka@sap.com>
There was a problem hiding this comment.
⚠️ Not ready to approve
The updated #target.datasource schema likely stops matching typical Grafana targets that include datasource.uid, which can prevent the Loki migration from applying even for valid Loki queries.
Pull request overview
This PR adjusts the Loki LogQL query migration CUE script to ensure it only migrates targets when the Grafana datasource explicitly declares datasource.type: "loki", preventing Prometheus (and other) targets from being incorrectly migrated as LokiLogQuery.
Changes:
- Removes the previous
datasource.type: "loki"constraint from#targetand replaces it with a runtime guard that requiresdatasource.typeto be present and equal to"loki". - Keeps datasource selection by UID when present, mapping it to a
LokiDatasourceselector.
File summaries
| File | Description |
|---|---|
loki/schemas/queries/loki-log-query/migrate/migrate.cue |
Adds a strict datasource.type == "loki" guard to prevent non-Loki targets from being migrated as Loki log queries. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 2
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| kind: "LokiDatasource" | ||
| name: #target.datasource.uid | ||
| if #target.datasource.type != _|_ if #target.datasource.type == "loki" { | ||
| kind: "LokiLogQuery" |
| datasource: { | ||
| type: "loki" | ||
| ... | ||
| type?: string | ||
| } |
Signed-off-by: gfierksap <gabor.fierka@sap.com>
Signed-off-by: gfierksap <gabor.fierka@sap.com>
|
@gfierkasap thnx for your contribution. Would you mind adding tests for this case? We have test for these cases in other plugins in the form of JSON files that validate input and output. e.g. https://github.com/perses/plugins/tree/main/prometheus/schemas/prometheus-time-series-query/migrate/tests/basic |
There was a problem hiding this comment.
⚠️ Not ready to approve
As implemented, the guard drops valid Loki targets that omit datasource.type (a pattern present in existing fixtures), which can lead to missed or incorrect query migrations.
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 2
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
| datasource: { | ||
| kind: "LokiDatasource" | ||
| name: #target.datasource.uid | ||
| if #target.datasource.type != _|_ if #target.datasource.type == "loki" { |
| if #target.datasource.type != _|_ if #target.datasource.type == "loki" { | ||
| kind: "LokiLogQuery" |
| #target: { | ||
| datasource: { | ||
| type: "loki" | ||
| type?: string |
There was a problem hiding this comment.
| type?: string | |
| type?: "loki" | |
| uid: string | |
| ... |
That sounds more accurate
Signed-off-by: gfierksap <gabor.fierka@sap.com>
@jgbernalp I tried adding a test case covering this scenario, but I find adding a negative case (like this one) a bit problematic, since I assume the expected.json should be empty. I can keep working on it, so far all I get is an error on the test case with the message |
Description
This PR is one part of the solution for perses/perses#4134
When migrating a Grafana dashboard to Perses, queries belonging to a Prometheus datasource were incorrectly migrated as LokiLogQuery with LokiDatasource.
This happened because the Loki migration script used a #target constraint to match targets with
datasource.type: "loki". However, due to how the migration engine works — it fills#targetwith the Grafana JSON via CUE's FillPath before unifying with the script — the type: "loki" constraint is not enforced as a guard. Instead, CUE merges it into the open struct, so the script matches any target that has an expr field and adatasource.uid, regardless of whetherdatasource.typeis present or correct.This particularly affected dashboards where datasource.type is absent from the Grafana JSON (only uid is set), which is valid Grafana syntax. In those cases the Loki script would match and win the migration — non-deterministically, since migrateQuery iterates over a Go map.