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

Dataplane: Enable dataplane for results of LongToWide #1057

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

kylebrandt
Copy link
Contributor

What this PR does / why we need it:

Setting the TypeVersion to greater than [0, 0] (along with Meta.Type being set) indicates that the produced
frame follows the dataplane contract (see https://grafana.com/developers/dataplane/ and https://github.com/grafana/dataplane).

	wideFrame.Meta.TypeVersion = FrameTypeVersion{0, 1}

Okay, so what will that do?
From a user perspective, there are no expected changes.

Certain datasources call LongWide (In particular, SQL datasources) when the query type in the UI is set to "time series". This sets the output of that call to be "dataplane compliant".

Two consumers will follow a different code path in this case (Server Side Expressions and Recorded Queries). From a user perspective things should change unless there is a bug in those code paths.

In SSE, the fork in the code Path is at:

https://github.com/grafana/grafana/blob/2136fd9a929334f39a371c2127c57b6d83abdf28/pkg/expr/converter.go#L32-L38

Special notes for your reviewer:
Since it is possible there could be an issue with a bug in the code that reads data plane data or something overlooked, I'm thinking through some role out possiblities (they have trade offs):

  • Just yolo it into the SDK
  • Change the signature of the function add argument (maybe Options{} so only done once, but breaking change, so this can be opted into)
  • Inverted approach - follow up with opt-out instructions (which would really just be overriding FrameTypeVersion{0, 1} back to FrameTypeVersion{0, 0} after calling LongToWide.

@kylebrandt kylebrandt added dataframe area/dataplane Dataplane Project (Data type contract) labels Aug 21, 2024
@kylebrandt kylebrandt requested a review from a team August 21, 2024 18:12
@gabor
Copy link
Contributor

gabor commented Aug 23, 2024

my recommendation would be to go with approach [1] (yolo 😁 ). i mean, it probably won't break and... even if we go with one of the more explicit ways, for example, let's say we go with [2]. this is used in grafana/sqlds, which is used in grafana/google-bigquery-datasource . so, grafana/sqlds will have to update to this change one day, but they cannot really test and make sure it does not break anything anyway, because it'll be grafana/google-bigquery-datasource where the breakage can happen.

that's why i think the simplest approach may be the best here. also, if a datasource sees that things started to break, they can just add a line that deletes the attribute from the dataframe, right?

WDYT?

@kylebrandt kylebrandt marked this pull request as ready for review August 23, 2024 11:27
@kylebrandt
Copy link
Contributor Author

they can just add a line that deletes the attribute from the dataframe, right?

Yes. Also leaning towards 1, but wanted to give some time for others (like you :-) ) to weigh in.

Copy link
Contributor

@gabor gabor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@kylebrandt kylebrandt merged commit 96b9aa7 into main Aug 26, 2024
3 checks passed
@kylebrandt kylebrandt deleted the longToWideDataplane branch August 26, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dataplane Dataplane Project (Data type contract) dataframe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants