feat: add cursor sync mechanism#304
Conversation
Codecov Report
@@ Coverage Diff @@
## master #304 +/- ##
=========================================
- Coverage 98.13% 98.04% -0.1%
=========================================
Files 37 37
Lines 2687 2714 +27
Branches 634 626 -8
=========================================
+ Hits 2637 2661 +24
- Misses 45 48 +3
Partials 5 5
Continue to review full report at Codecov.
|
|
Not sure if the issue elastic/kibana#14799 will be fixed by this, the sync still seems pretty slow. @markov00 any ideas? |
There was a problem hiding this comment.
I think we have to find a different way to implement that mechanism, the problem is the following:
the onCursorUpdateListener push out cursor positions as fast as it can, but we are limited by the slowness of setting the activeCursor value. This because we are continuously updating the specs on every cursor change triggering on every new setting change a computeChart.
With the refactoring I'm working on that will not be a big problem, but right now it still a limitation.
There is also another problem with the implementation: in Kibana we have to wire up this logic of cursorUpdate callback and activeCursor between completely separated elements, that can be a bit problematic: how will keep the state of the activeCursor?
I think that can be solved directly updating the state instead of going through the settings update. It can be achieved creating method on the Chart component that update the setCursorValue when called, and calling that from a another chart ref like the following:
// playground.tsx
export class Playground extends React.Component {
state = {
cursor: undefined,
}
ref1 = React.createRef<Chart>();
ref2 = React.createRef<Chart>();
ref3 = React.createRef<Chart>();
ref4 = React.createRef<Chart>();
componentDidMount() {
}
onCursorUpdate = (cursor: Cursor) => {
this.ref1.current.dispatchExternalCursorEvent(cursor);
this.ref2.current.dispatchExternalCursorEvent(cursor);
this.ref3.current.dispatchExternalCursorEvent(cursor);
this.ref4.current.dispatchExternalCursorEvent(cursor);
}
render() {
return (<React.Fragment>
{renderChart('1', this.ref1, KIBANA_METRICS.metrics.kibana_os_load[0].data.slice(0, 15), this.onCursorUpdate)
}
{renderChart('2', this.ref2, KIBANA_METRICS.metrics.kibana_os_load[1].data.slice(0, 15), this.onCursorUpdate)
}
{renderChart('3', this.ref3, KIBANA_METRICS.metrics.kibana_os_load[2].data.slice(15, 45), this.onCursorUpdate)
}
{renderChart('4', this.ref4, KIBANA_METRICS.metrics.kibana_os_load[1].data.slice(0, 100), this.onCursorUpdate)
}
</React.Fragment>)
}
}
function renderChart(key: string, ref: React.RefObject<Chart>, data: any, onCursorUpdate?: CursorUpdateListener) {
return (
<div key={key} className="chart">
<Chart ref={ref}>
<Settings debug={false} showLegend={true} onCursorUpdate={onCursorUpdate} />
<Axis
id={getAxisId('timestamp')}
title="timestamp"
position={Position.Bottom}
tickFormat={niceTimeFormatter([1555819200000, 1555905600000])}
/>
<Axis id={getAxisId('count')} title="count" position={Position.Left} tickFormat={(d) => d.toFixed(2)} />
<LineSeries
id={getSpecId('dataset A with long title')}
xScaleType={ScaleType.Time}
yScaleType={ScaleType.Linear}
data={data}
xAccessor={0}
lineSeriesStyle={{
line: {
stroke: 'red',
opacity: 1,
},
}}
yAccessors={[1]}
/>
<LineSeries
id={getSpecId('dataset B')}
xScaleType={ScaleType.Time}
yScaleType={ScaleType.Linear}
data={KIBANA_METRICS.metrics.kibana_os_load[1].data.slice(0, 15)}
xAccessor={0}
yAccessors={[1]}
stackAccessors={[0]}
/>
</Chart>
</div>);
}// chart.tsx
...
export class Chart extends React.Component<ChartProps> {
...
dispatchExternalCursorEvent(event: Cursor) {
if (event && event.chartId !== this.chartSpecStore.activeChartId) {
this.chartSpecStore.setCursorValue(event.value);
}
}
render() {
const { renderer, size, className } = this.props;
let containerStyle: CSSProperties;
if (size) {
....This is a just a possible implementation but works without going through the spec parsing and chart computing every time.
Refactor activeCursor logic to call method on Chart to update cursor and not rely on prop to Settings component.
|
@markov00 I updated the logic as we discussed. Much better performance 👍 I also added back the Demo |
markov00
left a comment
There was a problem hiding this comment.
I've added few comments that should be addressed before merge.
Do you think you can also add some more testing to have a green/positive diff coverage?
| headerFormatter?: TooltipValueFormatter; | ||
| } | ||
|
|
||
| export interface CursorEvent { |
There was a problem hiding this comment.
You missed to export that type from the main index.ts
There was a problem hiding this comment.
Can you also please add a bit more documentation for that interface?
# [9.1.0](v9.0.4...v9.1.0) (2019-08-14) ### Features * add cursor sync mechanism ([#304](#304)) ([c8c1d9d](c8c1d9d))
|
🎉 This PR is included in version 9.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [9.1.0](elastic/elastic-charts@v9.0.4...v9.1.0) (2019-08-14) ### Features * add cursor sync mechanism ([opensearch-project#304](elastic/elastic-charts#304)) ([3986dfc](elastic/elastic-charts@3986dfc))


Summary
Add
CursorUpdateListeneras props onSettingscomponent to allowChartconsumer to synchronize cursors across multipleCharts by callingdispatchExternalCursorEventon theChartref to update cursor value.Issue #83
Demo
Checklist
Charts sharing state)