Skip to content
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

New [Unified Alerts] API is broken #380

Closed
jmpdt opened this issue Nov 2, 2021 · 6 comments · Fixed by #387
Closed

New [Unified Alerts] API is broken #380

jmpdt opened this issue Nov 2, 2021 · 6 comments · Fixed by #387
Assignees

Comments

@jmpdt
Copy link

jmpdt commented Nov 2, 2021

For any alert created through the Grafana 8 alerting API, I am getting this error in the Grafana server logs:

t=2021-11-02T10:04:33-0400 lvl=eror msg="QueryData error: unable to parse json {\"dateTimeType\":\"DATETIME\",\"extrapolate\":true,\"format\":\"time_series\",\"formattedQuery\":\"SELECT $timeSeries as t, count() FROM $table WHERE $timeFilter GROUP BY t ORDER BY t\",\"hide\":false,\"intervalFactor\":1,\"intervalMs\":1000,\"maxDataPoints\":43200,\"query\":\"SELECT 1\\n\",\"rawQuery\":true,\"refId\":\"A\",\"round\":\"0s\",\"skip_comments\":true}. Error: json: cannot unmarshal bool into Go struct field Query.rawQuery of type string" logger=plugins.backend pluginId=vertamedia-clickhouse-datasource

My alert query is SELECT 1. I get the same error regardless of which table I choose as the data source. I can trigger this error message by clicking Reformat Query or Run Queries. In the Generated SQL dropdown, all that is displayed is true.

Grafana v8.2.2 and ClickHouse-Grafana v2.3.1

@Slach Slach self-assigned this Nov 10, 2021
@Slach
Copy link
Collaborator

Slach commented Nov 10, 2021

Sorry for the late reply
error message looks a little bit strange
rawQuery should be string even in last version of clickhouse grafana

if your query just SELECT 1 it should not work in any grafana version

clickhouse-grafana plugin expect timeseries query which contains first "t" (time) column and second column metric value or array with [category_field_value, metric_field_value]

@Slach
Copy link
Collaborator

Slach commented Nov 16, 2021

@jmpdt could you check your query again and please press to "Generated SQL" in panel
it should contains SQL query

@Slach Slach added this to the 2.4.0 milestone Nov 17, 2021
@Slach
Copy link
Collaborator

Slach commented Nov 17, 2021

@jmpdt thanks a lot for reporting.
I found root reason
this is cause grafana new feature unified alerting use POST /api/v1/eval directly instead of try to process query via datasource on client-side

I need time to think how to change code editor logic to allow us to change rawQuery from true to something applicable for query processing.

@Slach Slach changed the title Alerts API is broken New [Unified Alerts] API is broken Nov 18, 2021
@Slach Slach removed this from the 2.4.0 milestone Nov 18, 2021
@derN3rd
Copy link

derN3rd commented Nov 18, 2021

I need time to think how to change code editor logic to allow us to change rawQuery from true to something applicable for query processing.

I suggest to maybe have a similar solution as the default (my)sql datasource handles it:

  • Having functions/macros (helpers, i.e. for timeframes, grouping etc) that require to put in the columns. SQL example: $__timeGroupAlias(column,'5m') -> cast(cast(UNIX_TIMESTAMP(column)/(300) as signed)*300 as signed) AS "time"
  • When switching from Query Builder to Edit SQL he will generate the query, but you can't switch back to UI mode. Only one of them will be used at the same time
  • Using one default datatype (i.e. DateTime) in querys that is expected for Grafana (others like DateTime64 or Timestamp can be casted with toDateTime64 etc)

I guess this would solve most of the client side query building, right?

@jmpdt
Copy link
Author

jmpdt commented Nov 18, 2021

@Slach thanks for looking into this!

@Slach
Copy link
Collaborator

Slach commented Nov 18, 2021

@derN3rd
thanks a lot for suggestions,
Unfortunately, we can't apply it, cause clickhouse-grafana plugin have different logic than MySQL
we simplify first stage interface
and on first screen we just set up the table, database and datatime type and datetime field and date field (not required)

And we have many client macros, which we can't be resolved on client side before pass to backend.
Cause, currently, grafana doesn't provide way to access scopedVariables when use unified alerts and pass data directly to golang backend part to /api/v1/eval instead of try to process it via datasource.ts on browser side

Only one approach will work, try to implement full compatible with sql_query.ts and scanner.ts SQL parser on golang part, but it requires a lot of work and need provide lot of tests.

Slach added a commit to Altinity/grafana-plugin-repository that referenced this issue Nov 29, 2021
# 2.4.0 (2021-11-29)

## Enhancement:
* Add support for Grafana 8.x unified alerts, fix Altinity/clickhouse-grafana#380
* Add TLS support for backend alerts part of plugin Altinity/clickhouse-grafana#356 (comment)
* Add $naturalTimeSeries macro, look details in https://github.com/Vertamedia/clickhouse-grafana/pull/89/files#diff-cd9133eda7b58ef9c9264190db4534a1be53216edbda9ac57256fbd800368c03R383-R412
* Update golang-plugin-sdk-go to latest version
* Properly format Value in Table format, look details Altinity/clickhouse-grafana#379
* Remove toDateTime64 casting for column when time column is already DateTime64 to improve performance. Change test to ensure the casting is removed from the query, fix Altinity/clickhouse-grafana#360
* implements `$timeFilter64ByColumn(column_name)` macro, fix Altinity/clickhouse-grafana#343

## Fixes:

* implements properly GET and POST support for alert queries, fix Altinity/clickhouse-grafana#353
* SQL syntax highlight now works always, fix Altinity/clickhouse-grafana#174, fix Altinity/clickhouse-grafana#381
* fix Altinity/clickhouse-grafana#376,
* fix negative behavior for $perSecondColumns Altinity/clickhouse-grafana#337
* fix Altinity/clickhouse-grafana#374, ignore `--` inside quotas as comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants