search

grafana / athena-datasource

Apache License 2.0
38 stars 12 forks source link

Query Editor improvements: Update query with defaults on mount and filter out empty queries, prepare 2.13.3 #303

Closed idastambuk closed 10 months ago

idastambuk commented 10 months ago

In the old form styling, query={queryWithDefaults}is passed to the QueryEditor. I mistakenly replaced that with query={props.query} in the new styling code. This causes an error on Run query in new form styling if the user doesn't select any of the macros manually:

Screenshot 2023-11-26 at 21 21 13

Additionally, because of this, the autocomplete for the macros doesn't work, as we don't have defaults in the query to request the data.

The reason for this is that the connectionArgs (region, table etc.) aren't set to the default value.

Even the current solution in production isn't ideal, as the user has to change the query in the SQL editor for the defaults to get populated. This means if they run the query before editing sql, they will get the same error.

At first, I just replaced the SQL editor props to what we have in the old editor, however because of the bug mentioned above, I decided to refactor this a bit. Now we're calling onChange with the default query on mount. I've tested it and it works as expected, but I'd appreciate someone else trying to break it as well.

P.S. Ideally, we wouldn't have to set defaults anywhere in the Form, which is why getDefaultQuery method was implemented in datasource here in core Grafana. However, we decided we wouldn't implement that yet, as it wouldn't work for instances running Grafana<9.3 😔

idastambuk commented 10 months ago

querying works for me, but when i add a new panel to the dashboard, i still see the error. is that something that already exists or should i not be seeing that error at all?

You're right, this is an existing issue where Grafana triggers query() on opening a panel which causes the buildQuery to fail in any case. I added some code to filter out the query if the sql is empty. This way, we shouldn't see the reading region of undefined error at all.