grafana / sqlds

A package that assists writing SQL-driven datasources
Apache License 2.0
17 stars 12 forks source link

should we make time-series behavior closer to core-sql datasources? ( "metric" handling) #109

Open gabor opened 6 months ago

gabor commented 6 months ago

hi,

we have an issue reported for the bigquery-datasource (which uses sqlds), about a difference in behavior between bigquery-plugin and postgres. i'd like to discuss what we think about this, is this that we should change, add some optional-thing, or we should not have.

the postgres/mysq/mssql does a special handling for the following case:

for example, this query:

SELECT CURRENT_TIMESTAMP as time, 'sku' AS metric, 1 AS cost

the output from sqlds is a dataframe with 2 fields: the time-field and the value-field. this is about the value field. sqlds: value-field has: name=cost, labels={metric:sku} postgres: value-field has: name=sku, no labels

this will cause the legend-field in the visualisation to change: sqlds: legend has cost sku postgres: legend has sku

this can be a nicer visual representation in some cases (after all, if you only have 1 value-db-column, you don't care about it's name).

the postgres code: https://github.com/grafana/grafana/blob/86c618a6d62dcd2f33983fb0cecbad71781da8c3/pkg/tsdb/sqleng/sql_engine.go#L340-L354

i wonder what do you think about this. some arguments:

what do you think?

(one alternative is to do the change in the visualisation (timeseries in this case))

for reference, here's a list of various queries, and the legend-comparison between postgres and bigquery:


SELECT CURRENT_TIMESTAMP as time, 'sku' AS something, 1 AS cost

bigquery: cost sku postgres: cost sku


SELECT CURRENT_TIMESTAMP as time, 'sku' AS metric, 1 AS cost

bigquery: cost sku postgres: sku


SELECT CURRENT_TIMESTAMP as time, 'sku' AS metric, 'xyz' as thing, 1 AS cost

bigquery: cost {metric="sku", thing="xyz"} postgres: cost {metric="sku", thing="xyz"}


aangelisc commented 6 months ago

My personal thoughts are to make this behaviour the default with the ability to opt out. That will mean it isn't a breaking change but users can also switch to what may be the simpler option (not having the metric column handled in a special way). The only thing I'm unsure of is how exactly to describe this functionality.

gabor commented 6 months ago

@aangelisc thanks for the feedback.. but, could you clarify the "default with opt out" part? as in , should sqlds behave like postgres/mysql/mssql by default, and allow the user to opt out? i'm not against it, but i do think it's a breaking change.. right? (i'm probably misunderstanding something 😄 )

aangelisc commented 6 months ago

Sorry I should've said "opt-in". I think there are two options:

Does this make sense?

gabor commented 6 months ago

@aangelisc yes, clear now, thanks!

kylebrandt commented 6 months ago

The "special" metric handling case is there for backward compatibility as stated in the documentation:

For backward compatibility, there’s an exception to the above rule for queries that return three columns including a string column named metric. Instead of transforming the metric column into field labels, it becomes the field name, and then the series name is formatted as the value of the metric column. See the example with the metric column below.

When I created support for the "Long" format via LongToWide in SQL or SQL-Like data sources (ADX being the first), this backwards compatibility was not included (SQL Datasources: Allow multiple string/labels columns with time series). This caused issues for users (e.g. https://github.com/grafana/grafana/issues/35390), so the functionality for "metric" columns was restored (SQL: Fixes issues with showing value column name prefix in legends) to make it backwards compatible.

So I expecting removing this as a default behavior (at least for any existing queries) will be a breaking change that users will notice.

gabor commented 6 months ago

hi @kylebrandt thanks for the context... question.. do you consider this "special" handling a good thing or a bad thing? in other words, imagine you're creating the postgres datasource plugin today from scratch, so no backward-compat constrains... would you include this special-metric-handling code?

kylebrandt commented 6 months ago

hi @kylebrandt thanks for the context... question.. do you consider this "special" handling a good thing or a bad thing?

Bad.

in other words, imagine you're creating the postgres datasource plugin today from scratch, so no backward-compat constrains... would you include this special-metric-handling code?

No.

There might be some cases that are easier to query this way (if their tables happen to have this schema), but IIRC you can work around those with different queries. If we were to support it for convenience, it should be some sort of query option and not triggered by the contents/results of the query itself -- as that behavior can be surprising in a confusing way if a user is not aware of that backwards compatability exception to the normal logic.