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

feat(partition): drilldown #995

Merged
merged 14 commits into from
Feb 10, 2021
Merged

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Jan 28, 2021

Summary

Adds optional drilldown inside flame and icicle charts, an almost universal interaction for data exploration in observability and monitoring including APM, where the numerous layers and often, granular breakdown, may otherwise hide interesting details.

It not only magnifies the ever current interesting part during exploration, but also provides the space for in-rectangle labels. Otherwise, such charts are largely unreadable, due to the large number of items, and the legend's inherent difficulty in coping with it.

It's also an example for direct manipulation, where some change is effected by directly interacting with the components or shapes in the chart, rather than via external configuration. So it fits into the family of tooltip, shape and legend hover highlight.

drilldown

Checklist

Delete any items that are not applicable to this PR.

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility
  • Proper documentation or storybook story was added for features that require explanation or tutorials

It can be enhanced with

  • animation (transitioning)
  • alternative strategies similar to, or piggybacking on the legend options, ie. not just expand a node, but expand all identically named nodes (usually, the same monitored software function) in the same layer or in all layers
  • elimination of specific names, or values (or value ranges) eg. via context menu, as with "Exclude..." in common EDA tools
  • options for eg. hiding the 100% bottom layers, and if so, maybe initially showing a determinate number of layers eg. 10
  • better text formatting
  • reset options, eg. a transient reset button for when the user eliminated some branches
  • an indicator that tells the user that it's not currently showing all the data (maybe combined with the reset button)
  • maybe showing the currently selected path eg. above the legend

@monfera monfera added enhancement New feature or request discuss To be discussed wip work in progress :partition Partition/PieChart/Donut/Sunburst/Treemap chart related labels Jan 28, 2021
@@ -79,8 +82,10 @@ export function interactionsReducer(
},
};
case ON_MOUSE_DOWN:
const layerValues: LayerValue[] = getPickedShapesLayerValues(globalState)[0];
Copy link
Contributor Author

@monfera monfera Jan 28, 2021

Choose a reason for hiding this comment

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

I'm not sure this is the best approach, though

  • it's a very compact addition
  • fully reuses the existing data flow subgraph (DRY)
  • doesn't require anti-patterns(?) such as firing an action (eg. SET_DRILLDOWN) from within another action (ON_MOUSE_DOWN), or redux "middleware" and error prone thunks etc.
  • doesn't require asynchrony, timeout or some other fragile way to update
  • doesn't interfere with anything else linked to ON_MOUSE_DOWN, eg. the callbacks registered for shape selections go through the same code path as before

Disadvantages:

  • it still ties the gesture to the more semantic action; we should eventually be flexible about how to trigger drilldown; even an API callback could eventually do it
  • it broadens the reducer from state.interactions to the full global state so it feels a bit more coupled

It seems possible to currently solve this via selectors, ie. drilldown wouldn't be a state property; it'd be a selector. But

  • it'd have no information about the last action, because selectors receive only state, not the action
  • it'd not be possible to access the previous drilldown state: this is not currently done, but it's needed by tweening, which is very frequently done with this type of charts—while, with the current reducer action, it's trivial to retain the previous state:
      return {
        ...state,
        drilldown: layerValues ? layerValues[layerValues.length - 1].path.map((n) => n.value) : [],
        previousDrilldown: state.drilldown,
        ...

I ran into this tradeoff with the Kibana Canvas layout engine, which is rich in inherently stateful direct manipulation interactions (stuff being dragged/resized, snapping etc.) and used an approach there that worked out quite well; it uses a global inert state like our redux, but has some differences: it solved the issue of one action arising from some other, lower level actions.

@codecov-io
Copy link

codecov-io commented Jan 28, 2021

Codecov Report

Merging #995 (7e32a70) into master (514466f) will increase coverage by 0.00%.
The diff coverage is 96.42%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #995    +/-   ##
========================================
  Coverage   72.13%   72.14%            
========================================
  Files         355      371    +16     
  Lines       11160    10790   -370     
  Branches     2449     2225   -224     
========================================
- Hits         8050     7784   -266     
+ Misses       3095     2905   -190     
- Partials       15      101    +86     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/chart_types/partition_chart/layout/config.ts 53.33% <ø> (-1.22%) ⬇️
...types/partition_chart/layout/types/config_types.ts 100.00% <ø> (ø)
...tion_chart/layout/viewmodel/hierarchy_of_arrays.ts 100.00% <ø> (ø)
src/state/chart_state.ts 89.83% <ø> (+0.08%) ⬆️
...es/partition_chart/layout/utils/group_by_rollup.ts 86.86% <92.85%> (-1.02%) ⬇️
...hart_types/partition_chart/state/selectors/tree.ts 95.83% <100.00%> (+0.83%) ⬆️
src/state/reducers/interactions.ts 75.51% <100.00%> (-0.86%) ⬇️
src/state/selectors/get_legend_items_labels.ts 50.00% <0.00%> (-50.00%) ⬇️
src/utils/dimensions.ts 71.42% <0.00%> (-28.58%) ⬇️
src/components/no_results.tsx 50.00% <0.00%> (-25.00%) ⬇️
... and 211 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 514466f...7e32a70. Read the comment docs.

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, still need to play with it some later.

@monfera monfera requested a review from nickofthyme February 1, 2021 21:42
Base automatically changed from master to masterx February 2, 2021 23:10
Base automatically changed from masterx to master February 2, 2021 23:28
@monfera monfera removed the wip work in progress label Feb 3, 2021
@monfera
Copy link
Contributor Author

monfera commented Feb 3, 2021

jenkins retest this please

# Conflicts:
#	api/charts.api.md
#	src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.ts
#	src/chart_types/partition_chart/state/selectors/tree.ts
@monfera
Copy link
Contributor Author

monfera commented Feb 6, 2021

@markov00 7e32a70 addresses your legit concern that the interaction reducer shouldn't directly depend on chart type specific functions or selectors, it's now extracted it out, making distant future extensibility easier

# Conflicts:
#	src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.ts
#	src/chart_types/partition_chart/state/selectors/tree.ts
@monfera monfera requested a review from markov00 February 10, 2021 09:48
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Good to merge for me!

export interface AnimKeyframe {
time: number;
easingFunction: EasingFunction;
keyframeConfig: Partial<StaticConfig>;
}

export interface Config extends StaticConfig {
/** @alpha */
Copy link
Member

Choose a reason for hiding this comment

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

Moving things back to an alpha version after exporting this as public is not a good practice (we should mark this as breaking change) but I don't think anyone is using that config at the moment so we leave with it now

@monfera monfera merged commit 20bbdae into elastic:master Feb 10, 2021
github-actions bot pushed a commit that referenced this pull request Feb 15, 2021
# [24.6.0](v24.5.1...v24.6.0) (2021-02-15)

### Bug Fixes

* **legend:** width with scroll bar ([#1019](#1019)) ([45bd0d5](45bd0d5))

### Features

* sort values in actions by closest to cursor ([#1023](#1023)) ([e1da4e5](e1da4e5))
* **axis:** small multiples axis improvements ([#1004](#1004)) ([514466f](514466f))
* **partition:** drilldown ([#995](#995)) ([20bbdae](20bbdae))
@markov00
Copy link
Member

🎉 This PR is included in version 24.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Feb 15, 2021
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss To be discussed enhancement New feature or request :partition Partition/PieChart/Donut/Sunburst/Treemap chart related released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants