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

[graph-ng] add temporal DataFrame alignment/outerJoin & move null-asZero pass inside #29250

Merged
merged 7 commits into from Nov 21, 2020

Conversation

leeoniya
Copy link
Contributor

What this PR does / why we need it:

uPlot requires data to be aligned along x.

this changeset adds an alignment pass over a DataFrame[] and emits a single temporally-aligned DataFrame, in addition to a closured isGap null-testing function which uPlot needs to discern which nulls represent actual gaps from the original DataFrames, and which nulls are alignment artifacts.

the code is largely based on my alignment code used in example 3, here: https://leeoniya.github.io/uPlot/demos/path-gap-clip.html

additionally, the zero-fill pass is moved into the the alignment function, since it felt like it belonged there.

Special notes for your reviewer:

something that still needs to be addressed is the requirement for isGap on every initialized series to be updated just prior to any .setData() call, since these must always be kept in sync. something like this should be sufficient:

u.series.forEach(s => {
  s.isGap = newIsGap;
});

u.setData(newData);

@leeoniya leeoniya requested review from torkelo, ryantxu, dprokop and a team November 19, 2020 21:50
@CLAassistant
Copy link

CLAassistant commented Nov 19, 2020

CLA assistant check
All committers have signed the CLA.

@leeoniya leeoniya marked this pull request as draft November 19, 2020 21:52
@leeoniya leeoniya removed the request for review from a team November 19, 2020 21:53
@leeoniya
Copy link
Contributor Author

leeoniya commented Nov 19, 2020

not sure what's going on here. VSCode doesnt seem to have issues. maybe it's not upgrading uPlot to 1.4.2?

https://github.com/leeoniya/uPlot/blob/9ab576f67139567551e3a8dfc507b282f6b594fc/dist/uPlot.d.ts#L538-L543

Summary of all failing tests
FAIL packages/grafana-ui/src/components/uPlot/config/UPlotConfigBuilder.test.ts
  ● UPlotConfigBuilder › allows axes configuration

    TypeError: Cannot read property 'Side' of undefined

      57 |       label: 'test label',
      58 |       timeZone: 'browser',
    > 59 |       side: Axis.Side.Bottom,
         |                  ^
      60 |       isTime: false,
      61 |       formatValue: () => 'test value',
      62 |       grid: false,

      at Object.<anonymous> (packages/grafana-ui/src/components/uPlot/config/UPlotConfigBuilder.test.ts:59:18)

@torkelo
Copy link
Member

torkelo commented Nov 20, 2020

Wow! Did some tests,

50 series x 10000 data points = before 580ms, this PR: 68ms
50 series x 2000 data points = before 100ms, this PR 8ms

That's what I call a perf boost :)

The legend looks a bit wrong :)

Screenshot from 2020-11-20 10-17-36

@leeoniya
Copy link
Contributor Author

leeoniya commented Nov 20, 2020

@torkelo

how are you measuring?

10x is surprising to me. i'd expect something closer to 3x speedup, in line with raw Flot [1] unless there are some drastic perf cliffs when adding more series, or you guys have accidenrally introduced some slowdowns through flot customization.

it's also worth noting that starting with 1.4.0, uPlot uses a deferred/async rendering strategy via a microTask queue. so a simple console.timeEnd() will no longer give you the full render time. you have to queue it up either via window.queueMicrotask or a resolved promise:

https://github.com/leeoniya/uPlot/blob/master/bench/uPlot.html#L135

i'd like to perf test this myself but yarn start always emits unoptimized dev builds, so i don't know how to get representative numbers for the overall improvement.

i'll dig into the Flot panel a bit today and add a few console.time calls to see. maybe some of the improvement you're seeing is from other panel-related changes and not just the graph?

[1] https://leeoniya.github.io/uPlot/demos/multi-bars.html

@torkelo
Copy link
Member

torkelo commented Nov 20, 2020

I used the performance capture feature in chrome dev tools

Screenshot from 2020-11-20 16-03-50

@torkelo
Copy link
Member

torkelo commented Nov 20, 2020

you can use yarn build to get an optimized build

@leeoniya
Copy link
Contributor Author

@torkelo sorry, i think we've been talking past each other. i thought you were comparing to Flot, not the previous alignment strategy. in retrospect it should have been obvious.

i think the main speedup here probably comes from the fast-path exit for single dataframes (which are already naturally aligned):

if (tables.length === 1) {
return {
data: tables[0],
isGap: () => true,
};
}

@leeoniya leeoniya marked this pull request as ready for review November 21, 2020 05:24
Copy link
Member

@ryantxu ryantxu left a comment

Choose a reason for hiding this comment

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

Names look good now -- still something funky with enums and uPlot typescript, but let's fix that in a different issue

@leeoniya leeoniya merged commit 917b5c5 into master Nov 21, 2020
@leeoniya leeoniya deleted the graph-ng-align-frames branch November 21, 2020 05:49
xlson added a commit to AgnesToulet/grafana that referenced this pull request Nov 24, 2020
* master: (71 commits)
  Security: Fixes minor security issue with alert notification webhooks that allowed GET & DELETE requests grafana#29330
  Chore: Bump storybook to v6 (grafana#28926)
  ReleaseNotes: Updates release notes link in package.json (master) (grafana#29329)
  Docs: Accurately reflecting available variables (grafana#29302)
  Heatmap: Fixes issue introduced by new eventbus (grafana#29322)
  Dashboard Schemas (grafana#28793)
  devenv: Add docker load test which authenticates with API key (grafana#28905)
  Login: Fixes redirect url encoding issues of # %23 being unencoded after login (grafana#29299)
  InfluxDB: update flux library and support boolean label values (grafana#29310)
  Explore/Logs: Update Parsed fields to Detected fields (grafana#28881)
  GraphNG: Init refactorings and fixes (grafana#29275)
  fixing a broken relref link (grafana#29312)
  Drone: Upgrade build pipeline tool (grafana#29308)
  decreasing frontend docs threshold. (grafana#29304)
  Docker: update docker root group docs and docker image (grafana#29222)
  WebhookNotifier: Convert tests away from goconvey (grafana#29291)
  Annotations: fixing so when changing annotations query links submenu will be updated. (grafana#28990)
  [graph-ng] add temporal DataFrame alignment/outerJoin & move null-asZero pass inside (grafana#29250)
  Dashboard: Fixes kiosk state after being redirected to login page and back (grafana#29273)
  make it possible to hide change password link in profile menu (grafana#29246)
  ...
@leeoniya leeoniya mentioned this pull request Nov 27, 2020
55 tasks
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