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

Make d3 place nicely with object values #62004

Merged
merged 3 commits into from
Apr 2, 2020

Conversation

flash1293
Copy link
Contributor

Summary

Fixes #57122

Currently range aggregations (ip range, date range) are not rendering correctly when put on the x axis, because d3 scales don't play well with objects as values (a range object looks like this: { from: 123, to: 456 }).

When a new ordinal scale is created, it uses a "D3 map" internally to store the set of values and its indices. All values are casted to string because its using a regular JS object as storage mechanism. This means an object like { from: 123, to: 456 } becomes "[object Object]" - as all values will be mapped to the same string, there will be only a single value on the x axis.

Previously to #48090 this worked, because the formatter was applied to the object before the data table got passed to the visualization. However that's problematic because the visualization now never knows about the raw value behind a data point and can't reliably apply filters: #40334

To fix the problem, this PR overwrites the toString function of the complex objects in the data table, allowing the d3 map to work as expected.

Checklist

@flash1293 flash1293 marked this pull request as ready for review April 1, 2020 08:43
@flash1293 flash1293 requested a review from a team April 1, 2020 08:43
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Apr 1, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@flash1293 flash1293 requested a review from markov00 April 1, 2020 08:44
@timroes timroes requested a review from majagrubic April 1, 2020 10:22
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

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.

Code LGTM.
Tested locally on Mac OS 10.14 Chrome 80:

  • checked vertical bar charts with various aggregation (Date Range, Ip Range, Range)
  • checked horizontal bars with the same aggs

Note: not sure if this is caused by these changes or not, but when creating a vertical bar with a Date Range, Ip Range or Range agg like the one mentioned above, if you change the X axis position to left, the resulting chart loose the top most axis label. The label shows up only disabling the Filter labels mysterious flag 👻 :)
If this is a different issue, let's consider this approved for me

Update: the above issue is probably the same as #21589

@flash1293
Copy link
Contributor Author

The label shows up only disabling the Filter labels mysterious flag 👻 :)

Aah, here we go again. I think it's not related, just another problem. I will look into it.

@flash1293
Copy link
Contributor Author

@markov00 It also happens without this change, but I think I figured it out. Will prepare a separate PR for this.

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Tested on Mac OS in Chrome with sample logs index pattern. Created vertical bar with date range aggregation, works as expected. Haven't tested other visualizations.

@flash1293 flash1293 merged commit 61f2626 into elastic:master Apr 2, 2020
@flash1293 flash1293 deleted the fix/date-range-problem branch April 2, 2020 16:12
flash1293 added a commit to flash1293/kibana that referenced this pull request Apr 2, 2020
flash1293 added a commit to flash1293/kibana that referenced this pull request Apr 2, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 2, 2020
* master:
  Switch to embeddable factory interface with optional override (elastic#61165)
  fix text error in diagrams (elastic#62101)
  [Index management] Prepare support Index template V2 format (elastic#61588)
  Updates dashboard images (elastic#62011)
  [Maps] remove MapBounds type (elastic#62332)
  [Uptime] Convert anomaly Jobs name to lowercase to comply with… (elastic#62293)
  Make d3 place nicely with object values (elastic#62004)
  EMT-287: update schema with elastic agent id (elastic#62252)
  [Maps] fix replaceLayerList to handle case where map is not intialized (elastic#62202)
  Remove support for deprecated xpack.telemetry configurations (elastic#51142)
  [Uptime] Remove static constant for index name completely (elastic#62256)
  [APM] E2E: install dependencies for vanilla workspaces (elastic#62178)
  [backport] Bump to 5.1.3 (elastic#62286)
  Show server name in Remote Cluster detail panel (elastic#62250)
  Rename some alert types (elastic#61693)
  changing duration type to ms, s, m (elastic#62265)
  [ML] Clear Kibana index pattern cache on creation or form reset. (elastic#62184)
  Move `src/legacy/server/index_patterns` to data plugin (server) (Remove step) (elastic#61618)
  [NP] Remove IndexedArray usage in Schemas (elastic#61410)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Vislib Vislib chart implementation release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with Time Range visualization
5 participants