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

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

Open
gabor opened this issue Feb 16, 2024 · 7 comments

Comments

@gabor
Copy link
Contributor

gabor commented Feb 16, 2024

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:

  • format=time_series
  • exactly 3 db-columns
  • the string-column is named metric

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:

  • pro:
    • this may be a very common time-series query (don't know), so making it more ergonomical may be a good thing
    • we already have some assumptions about the data in the format=time_series case
  • con:
    • not doing it makes the behavior simpler, easier to understand, this special-behavior is not really discoverable
    • changing the behavior breaks backward compatibility
    • there is a workaround: use a transform (labels-to-fields or organize-by-name)

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
Copy link

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
Copy link
Contributor Author

gabor commented Feb 16, 2024

@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
Copy link

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

  • sqlds can continue to behave as it does but allow the user to opt-out for the more standard (?) behaviour seen in the core sql data sources
  • The core sql data sources continue to behave as they currently do with the ability to opt out of the additional logic they currently have as default.

Does this make sense?

@gabor
Copy link
Contributor Author

gabor commented Feb 16, 2024

@aangelisc yes, clear now, thanks!

@kylebrandt
Copy link

kylebrandt commented Feb 16, 2024

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. grafana/grafana#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
Copy link
Contributor Author

gabor commented Feb 16, 2024

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
Copy link

kylebrandt commented Feb 16, 2024

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.

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

No branches or pull requests

3 participants