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.

Sign up for GitHub

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

Jump to bottom

[XY Chart] Fix "No data to display" error when using IP range aggregation to split series #93024

Merged
merged 4 commits into from
Mar 3, 2021
Merged

[XY Chart] Fix "No data to display" error when using IP range aggregation to split series #93024

merged 4 commits into from
Mar 3, 2021

Conversation

DianaDerevyankina
Copy link
Contributor

Closes #92835

Summary

Fix "No data to display" error for XY chart.

For IP range aggregation visData contains data with range object instead of simple string or number, so series should have complex accessors in this case.
image

Added an IP range for aggregation range types check.

For maintainers

@DianaDerevyankina DianaDerevyankina added Feature:XYAxis XY-Axis charts (bar, area, line) Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.1 labels Mar 1, 2021
@DianaDerevyankina DianaDerevyankina self-assigned this Mar 1, 2021
alexwizp
alexwizp approved these changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you

@DianaDerevyankina DianaDerevyankina marked this pull request as ready for review March 1, 2021 14:26
@DianaDerevyankina DianaDerevyankina requested a review from a team March 1, 2021 14:26
@elasticmachine
Copy link
Contributor

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

stratoula
stratoula approved these changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

LGTM, tested it locally, now the ip range works either with split series or as an aggregation on the x-axis

@stratoula
Copy link
Contributor

@elasticmachine run elasticsearch-ci/docs

nickofthyme
nickofthyme approved these changes Mar 1, 2021
View reviewed changes
src/plugins/vis_type_xy/public/utils/accessors.tsx
@@ -27,7 +27,7 @@ const getFieldName = (fieldName: string, index?: number) => {
};

export const isRangeAggType = (type: string | null) =>
type === BUCKET_TYPES.DATE_RANGE || type === BUCKET_TYPES.RANGE;
type === BUCKET_TYPES.DATE_RANGE || type === BUCKET_TYPES.RANGE || type === BUCKET_TYPES.IP_RANGE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice fix!

wylieconlon
wylieconlon reviewed Mar 1, 2021
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Can you add some tests for this?

wylieconlon
wylieconlon suggested changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Marking as change requested for three reasons:

  1. No tests
  2. The title of the PR does not meet our release note standards. If this title appeared in our release notes, users might think that you've removed the IP range split. Instead, the title should answer the question "what can I do now that was broken before?"
  3. The PR labels indicate that this is for 7.12.1, which is only true if you merge this after we release 7.12- anything merged before 7.12 is released onto the 7.12 branch will be 7.12.0

@stratoula stratoula added v7.12.0 and removed v7.12.1 labels Mar 2, 2021
@stratoula
Copy link
Contributor

@wylieconlon thank you for your review. I agree, it is a great opportunity to add tests, I also missed the label, I changed it. We plan to merge it for 7.12.0. As this is a bug introduced with the new xy plugin that will be released on 7.12.0 too, this PR won't appear on the release notes as the bug "never existed". Nevertheless, the title is confusing and needs to be changed 👍

@DianaDerevyankina DianaDerevyankina changed the title Visualize: Can't use ip range to split series in xy chart [XY Chart] Fix "No data to display" error when using IP range aggregation to split series Mar 2, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeXy 119.8KB 119.9KB +46.0B

History

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

cc @dziyanadzeraviankina

@stratoula
Copy link
Contributor

@elasticmachine run elasticsearch-ci/docs

wylieconlon
wylieconlon approved these changes Mar 2, 2021
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM! Please add the 7.13.0 tag as well.

@DianaDerevyankina DianaDerevyankina merged commit 7bd1f6f into elastic:master Mar 3, 2021
@DianaDerevyankina DianaDerevyankina mentioned this pull request Mar 3, 2021
Merged
DianaDerevyankina added a commit to DianaDerevyankina/kibana that referenced this pull request Mar 3, 2021
@DianaDerevyankina
[XY Chart] Fix "No data to display" error when using IP range aggrega…
9256946 
…tion to split series (elastic#93024)

* Visualize: Can't use ip range to split series in xy chart

* Refactor accessors.tsx

* Revert "Refactor accessors.tsx"

This reverts commit f2b088e.

* Add accessors.test to cover getComplexAccessor function
DianaDerevyankina added a commit to DianaDerevyankina/kibana that referenced this pull request Mar 3, 2021
@DianaDerevyankina
[XY Chart] Fix "No data to display" error when using IP range aggrega…
3d3ae39 
…tion to split series (elastic#93024)

* Visualize: Can't use ip range to split series in xy chart

* Refactor accessors.tsx

* Revert "Refactor accessors.tsx"

This reverts commit f2b088e.

* Add accessors.test to cover getComplexAccessor function
@DianaDerevyankina DianaDerevyankina mentioned this pull request Mar 3, 2021
Merged
DianaDerevyankina added a commit that referenced this pull request Mar 3, 2021
@DianaDerevyankina
[XY Chart] Fix "No data to display" error when using IP range aggrega…
b7a215d 
…tion to split series (#93024) (#93368)

* Visualize: Can't use ip range to split series in xy chart

* Refactor accessors.tsx

* Revert "Refactor accessors.tsx"

This reverts commit f2b088e.

* Add accessors.test to cover getComplexAccessor function
DianaDerevyankina added a commit that referenced this pull request Mar 3, 2021
@DianaDerevyankina
[XY Chart] Fix "No data to display" error when using IP range aggrega…
2a82391 
…tion to split series (#93024) (#93367)

* Visualize: Can't use ip range to split series in xy chart

* Refactor accessors.tsx

* Revert "Refactor accessors.tsx"

This reverts commit f2b088e.

* Add accessors.test to cover getComplexAccessor function
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 3, 2021
@gmmorris
Merge branch 'master' into task-manager/docs-monitoring
b0c15d5 
* master: (45 commits)
  Add outcome of node scripts/build_api_docs (elastic#93399)
  [Lens] fix long field name on field stats panel doesn't wrap (elastic#93279)
  [Bug] Fix filter creation for numeric scripted fields in Discover (elastic#93224)
  [uptime] Fix anomaly alert edit (elastic#93025)
  Consolidate @babel/* packages and use latest compatible version (elastic#93264)
  [Search Embeddable] Add highlighting when searching (elastic#93178)
  [APM] Add missing bottom border to header (elastic#93179)
  [CI] No longer collect APM span stack traces (elastic#93263)
  [XY Chart] Fix "No data to display" error when using IP range aggregation to split series (elastic#93024)
  update generated public api docs
  API DOCS Step 3/3 (elastic#92929)
  chore(NA): look for bazel packages on npm_module folder during distributable build (elastic#93262)
  rename advanced setting ml:fileDataVisualizerMaxFileSize to fileUpload:maxFileSize and increase max geojson upload size to 1GB (elastic#92620)
  [kbn/optimizer] allow customizing the limits path from the script (elastic#93153)
  [Alerting][Docs] Adding template for documenting alert and action types (elastic#92830)
  [jenkins] convert baseline capture job to use tasks (elastic#93288)
  removing the linked issue in comments from PR (elastic#93303)
  chore(NA): do not include fs within a storybook build (elastic#93294)
  [Maps] Update Map extent queries to use bounding box logic for both point and shape queries (elastic#93156)
  Add searchDuration to EQL and Threshold rules (elastic#93149)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexwizp alexwizp alexwizp approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

@stratoula stratoula stratoula approved these changes

@wylieconlon wylieconlon wylieconlon approved these changes

Assignees

@DianaDerevyankina DianaDerevyankina

Labels
Feature:XYAxis XY-Axis charts (bar, area, line) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v7.13.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Visualize: Can't use ip range to split series in xy chart
7 participants
@DianaDerevyankina @elasticmachine @stratoula @kibanamachine @wylieconlon @nickofthyme @alexwizp