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

Streaming: improve JSDocs for DataSourceAPI streaming support #18672

Merged
merged 9 commits into from
Aug 22, 2019

Conversation

ryantxu
Copy link
Member

@ryantxu ryantxu commented Aug 22, 2019

After discussing streaming support with @aocenas, it is clear we are missing good docs somewhere :)

This is an attempt to add better JSDocs. We likely need a blog post and more, but I hope to do that when we have a more coherent streaming solution. ie, same thing works in grafana and explore

@@ -295,7 +313,7 @@ export interface ExploreStartPageProps {
*/
export type LegacyResponseData = TimeSeries | TableData | any;

export type DataQueryResponseData = DataFrameDTO | LegacyResponseData;
export type DataQueryResponseData = DataFrame | DataFrameDTO | LegacyResponseData;
Copy link
Member Author

Choose a reason for hiding this comment

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

unrelated... but I added it while I'm looking

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be DataFrameHelper as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, DataFrameHelper implements DataFrame -- it is just a helper ;)

Copy link
Contributor

@hugohaggmark hugohaggmark left a comment

Choose a reason for hiding this comment

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

LGTM, made some minor comments.

* 4. {key:K2, data:[C4]} >> PanelData: [A,B3,C4]
*
* NOTE: that PanelData will report a `Done` state until all
* unique keys have returned with either `Error` or `Done` state.
Copy link
Member

Choose a reason for hiding this comment

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

Whoa, are you saying that this is basically a stream of streams that you have to manually check by the key? Honestly did not see any code like that so far so not sure what to make from this. It seems like all of this is meant not to be used directly but only through PanelQueryRunner or something but that isn't the case right now so all these assumptions could be broken somewhere.

Copy link
Member Author

@ryantxu ryantxu Aug 22, 2019

Choose a reason for hiding this comment

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

assumptions could be broken

well obviously!

stream of streams

remember that the query argument is not a single query. it is a request with a query[] (targets), given that each query can return an array of data, that is how this mess gets made

* then you can ignore the `observer` entirely.
*
* When streaming behavior is required, the Promise can return at any time
* with empty or partial data in the response and optionally a state.
Copy link
Member

Choose a reason for hiding this comment

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

This I think is broken here https://github.com/grafana/grafana/pull/16676/files#r316658833. even though that is older PR I feel like these kind of errors are too easy to make here.

* Data may not be known yet
* The streaming events return entire DataFrames. The DataSource
* sending the events is responcible for truncating any growing lists
* most likely to the requested `maxDataPoints`
Copy link
Member

Choose a reason for hiding this comment

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

No idea here why this is array of dataframes and not just a dataframe. At least on Explore side this seems to be flattened anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

a query often returns many DataFrames. and again remember, the existing query request contains a list of queries, not a single query

Copy link
Member

Choose a reason for hiding this comment

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

This cannot be flattened in Explore, as a time series is presented as a single DataFrame (with time and value field), and Explore needs to show more than one time series :) A single query can return many series (explore also supports multiple queries which of course can return multiple series (ie DataFrames)

ryantxu and others added 2 commits August 22, 2019 07:01
Co-Authored-By: Hugo Häggmark <hugo.haggmark@gmail.com>
Co-Authored-By: Hugo Häggmark <hugo.haggmark@gmail.com>
@ryantxu ryantxu merged commit ca77836 into grafana:master Aug 22, 2019
@ryantxu
Copy link
Member Author

ryantxu commented Aug 22, 2019

@aocenas I added these docs as it defines the current behavior, I'll take a stab at an updated API that incorporates your feedback

@ryantxu ryantxu deleted the streaming-confusion branch August 22, 2019 17:01
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 22, 2019
* grafana/master:
  Streaming: improve JSDocs for DataSourceAPI streaming support (grafana#18672)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 23, 2019
* grafana/master:
  Streaming: improve JSDocs for DataSourceAPI streaming support (grafana#18672)
  TimeSrv: Enable value time windowing in TimeSrv (grafana#18636)
  Explore: Fixes so Show context shows results again (grafana#18675)
  Graph: Updated y-axis ticks test dashboard (grafana#18677)
  Add typings to package.json in packages (grafana#18640)
  Plugins: better warning when plugins fail to load (grafana#18671)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 26, 2019
* grafana/master:
  Heatmap: Add Cividis and Turbo color schemes (grafana#18710)
  Units: add counts/sec (cps) and counts/min (cpm) in Throughput (grafana#18702)
  Dev Docker: Use golang:1.12.9-alpine to prevent glibc mismatch. (grafana#18701)
  Docs: Fix broken link for the Grafana on RHEL or Ubuntu tutorial (grafana#18697)
  Fixes several usability issues with QueryField component  (grafana#18681)
  convert teams section of user profile to react (grafana#18633)
  Singlestat/Gauge/BarGauge: Improvements to decimals logic and added test dashboard (grafana#18676)
  Emails: Change text (grafana#18683)
  Streaming: improve JSDocs for DataSourceAPI streaming support (grafana#18672)
  TimeSrv: Enable value time windowing in TimeSrv (grafana#18636)
  Explore: Fixes so Show context shows results again (grafana#18675)
  Graph: Updated y-axis ticks test dashboard (grafana#18677)
  Add typings to package.json in packages (grafana#18640)
  Plugins: better warning when plugins fail to load (grafana#18671)
  SingleStat2: save options to defaults not override (grafana#18666)
  Packages: Fix path import from grafana/data (grafana#18667)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 26, 2019
* grafana/master:
  @grafana/data: improve the CircularVector api (grafana#18716)
  QueryEditor: check if optional func toggleEditorMode is provided (grafana#18705)
  Emails: remove the yarn.lock (grafana#18724)
  OAuth: Support JMES path lookup when retrieving user email (grafana#14683)
  Emails: resurrect template notification (grafana#18686)
  Email: add reply-to and direct attachment (grafana#18715)
  Dashboard: Adds Logs Panel (alpha) as visualization option for Dashboards (grafana#18641)
  Heatmap: Add Cividis and Turbo color schemes (grafana#18710)
  Units: add counts/sec (cps) and counts/min (cpm) in Throughput (grafana#18702)
  Dev Docker: Use golang:1.12.9-alpine to prevent glibc mismatch. (grafana#18701)
  Docs: Fix broken link for the Grafana on RHEL or Ubuntu tutorial (grafana#18697)
  Fixes several usability issues with QueryField component  (grafana#18681)
  convert teams section of user profile to react (grafana#18633)
  Singlestat/Gauge/BarGauge: Improvements to decimals logic and added test dashboard (grafana#18676)
  Emails: Change text (grafana#18683)
  Streaming: improve JSDocs for DataSourceAPI streaming support (grafana#18672)
  TimeSrv: Enable value time windowing in TimeSrv (grafana#18636)
  Explore: Fixes so Show context shows results again (grafana#18675)
  Graph: Updated y-axis ticks test dashboard (grafana#18677)
  Add typings to package.json in packages (grafana#18640)
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 this pull request may close these issues.

None yet

4 participants