[Lens] Add specific IP and Range/Interval sorting to datatable #87006

dej611 · 2020-12-29T19:05:03Z

Summary

Fixes: #85431, enhances #84435

This PR adds a specific logic to sort IP and Range type of data into the custom datatable sorting.

The previous sorting function has been rewritten for performance reason: the lodash orderBy was using only a single field per time and to perform an IP sorting logic it should have accepted 8 functions (one per IPv6 part/block) or perform some hacky manipulation.

Some initial unit testing has been added for the sorting logic, with particular focus on IP and ranges.

How Range sorting works

The range sorting algorithm works as follow:

Make explicit each range data object (i.e. {gte: 1} => {gte: 1, lt: Infinity})
Perform a comparison on the gte
if gte is identical then compare the lt value
Flip the direction using a +1 / -1 multiplier

How IP sorting works

In order to achieve the IP sorting a new dependency was added ( ipaddr.js - note that was already a subdependency of webpack-web-server), to support IPv6 parsing in its multiple form.

The IP sorting algorithm works as follow:

parse the string and check if it's a valid IP address ( IPv4 or IPv6 format and ranges )
- if not valid return a mock value that makes it sure to be the minimal value in the array (i.e. Other value) (opinion)
transform the parsed IP into a IPv6 address (any IPv4 can be mapped into a IPv6, but it is not true the other way around)
convert each IPv6 part/block into a 16bit integer and compare them. Stop at the first difference.
in case the two addresses are identical (diff = 0 for each part/block) then compare the string length: the shorter form should be considered "smaller" than the longer form ( opinion )
Flip the direction using a +1 / -1 multiplier

Checklist

Unit or functional tests were updated or added to match the most common scenarios

dej611 · 2020-12-30T10:22:14Z

Explored the option to embed this code into the data plugin, but I saw some more piping required in order to use it into Lens datatable in this scenario. We could consider to move it later once the feature will be shared with other plugins (for instance, thinking of Visualize or Discover)

elasticmachine · 2020-12-30T10:32:02Z

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

wylieconlon

Overall I think this code is simple and makes sense. Left a comment about the behavior of IP sorting which I think we should change, but LGTM.

wylieconlon · 2020-12-30T16:16:46Z

x-pack/plugins/lens/public/datatable_visualization/sorting.test.tsx

+      it(`should provide the IP criteria for IP values (mixed values with invalid "Other" field) - ${direction}`, () => {
+        testSorting({
+          input: ['fc00::123', '192.168.1.50', '10.0.1.76', '8.8.8.8', '::1', 'Other'],
+          output: ['Other', '::1', '8.8.8.8', '10.0.1.76', '192.168.1.50', 'fc00::123'],


I noticed that Other was sorted to the beginning of the list- I kind of expected it to be sorted in a special way, always being at the end of the list regardless of the sort order. What do you think?

Possible, sure.
It's really up to us to decide a value out of the range I guess: the current implementation, as I saw it, mimics the "blank" values on numeric fields, which are casted to 0.
Putting it at last should be straightforward as it knows already the direction.

wylieconlon · 2020-12-30T16:19:05Z

x-pack/plugins/lens/public/datatable_visualization/sorting.tsx

+  if (!ipaddr.isValid(ip)) {
+    // for non valid IPs have the same behaviour as for now (we assume it's only the "Other" string)
+    // create a mock object which has all -Infinity values to be the last item
+    return { parts: Array(8).fill(-Infinity) };


I think the result of this is that Other is always shown at the beginning of the list. Can we push it to the end instead, since I would want to show IPs in ascending order?

wylieconlon · 2020-12-30T16:19:47Z

x-pack/plugins/lens/public/datatable_visualization/sorting.tsx

+      diff = ipA.parts[i] - ipB.parts[i];
+    }
+
+    // in case of same address but written in different styles, sort by string length


dej611 · 2020-12-30T18:11:59Z

Implemented the "always last" logic for strings in the IP sorter:

tsullivan · 2020-12-30T23:46:48Z

Hi, I'm taking a look at the changes. In the meantime, the PR description needs a clearly written Release Note since this is an enhancement :)

tsullivan · 2020-12-31T00:38:44Z

Hi, the IP field sorting changes look good!

When I tested sorting range field types, I wasn't able to see the field in Lens. In Management > Index Patterns, the field type is labelled as unknown:

To test the range field type, is a setup step needed?

wylieconlon · 2020-12-31T18:18:48Z

Hi @tsullivan, Kibana doesn't support range types (although I have a separate PR for it here #76971). Marco was talking about the ranges aggregation, which you can set in Lens by going to Horizontal axis -> Intervals -> Custom intervals

flash1293 · 2021-01-04T12:54:11Z

@elasticmachine merge upstream

flash1293 · 2021-01-05T09:11:33Z

@elasticmachine merge upstream

flash1293

Tested and works as expected, LGTM. Left one small nit about the code

flash1293 · 2021-01-05T09:04:13Z

x-pack/plugins/lens/public/datatable_visualization/sorting.tsx

+  }
+  // use a string sorter for the rest
+  return (rowA: Record<string, unknown>, rowB: Record<string, unknown>) => {
+    const aString = formatter?.convert(rowA[sortBy]);


Is it possible there is no formatter? The types indicate otherwise, so if the will always be a formatter, we can remove the optional chaining - if it can actually be undefined, we should do something like this: aString = formatter ? formatter.convert(rowA[sortBy]) : rowA[sortBy], otherwise it will just compare undefined with undefined

The formatFactory should ensure each column has a formatter:

kibana/x-pack/plugins/lens/public/datatable_visualization/expression.tsx

Lines 128 to 130 in c545b32

firstTable.columns.forEach((column) => {

formatters[column.id] = formatFactory(column.meta?.params);

});

I'll remove the optional chaining here.

dej611 · 2021-01-11T09:23:01Z

@elasticmachine merge upstream

…e-ip-sorting

…ana into feature/lens/table-ip-sorting

kibanamachine · 2021-01-11T13:02:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 251a444

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	463	465	+2

Async chunks

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

id	before	after	diff
`enterpriseSearch`	1.8MB	1.8MB	+20.0B
`lens`	1.0MB	1.0MB	+17.1KB
total			+17.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`upgradeAssistant`	60.1KB	60.1KB	+20.0B

History

💚 Build #97822 succeeded c079b89
💛 Build #96879 was flaky ca5c970
💚 Build #96612 succeeded e7c610a
💚 Build #96454 succeeded 5fafb2c
💚 Build #96397 succeeded ea7e910

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

…ic#87006) Co-authored-by: Kibana Machine <[email protected]>

…87006) (#87975) Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

dej611 added 2 commits December 29, 2020 19:52

➕ Add new IP parser dependency

2693006

✨ Implemented ip and ranges sorting into Lens datatable

a552300

dej611 added Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0 labels Dec 30, 2020

✅ Add more tests for strings and booleans

ea7e910

dej611 marked this pull request as ready for review December 30, 2020 10:32

dej611 requested a review from a team December 30, 2020 10:32

wylieconlon approved these changes Dec 30, 2020

View reviewed changes

dej611 added 2 commits December 30, 2020 19:05

✨ Implement always last logic for external values

e768eb7

♻️ Refactor a bit variables to reduce type casting around

5fafb2c

Merge branch 'master' into feature/lens/table-ip-sorting

e7c610a

Merge branch 'master' into feature/lens/table-ip-sorting

ca5c970

flash1293 approved these changes Jan 5, 2021

View reviewed changes

kibanamachine and others added 4 commits January 11, 2021 04:23

Merge branch 'master' into feature/lens/table-ip-sorting

c079b89

Merge remote-tracking branch 'upstream/master' into feature/lens/tabl…

8d97bbe

…e-ip-sorting

👌 Remove optional chaining

9e6a262

Merge branch 'feature/lens/table-ip-sorting' of github.com:dej611/kib…

251a444

…ana into feature/lens/table-ip-sorting

dej611 merged commit 132e851 into elastic:master Jan 11, 2021

dej611 deleted the feature/lens/table-ip-sorting branch January 11, 2021 15:51

dej611 mentioned this pull request Jan 12, 2021

[7.x] [Lens] Add specific IP and Range/Interval sorting to datatable (#87006) #87975

Merged

dej611 added a commit to dej611/kibana that referenced this pull request Jan 12, 2021

[Lens] Add specific IP and Range/Interval sorting to datatable (elast…

aa36e63

…ic#87006) Co-authored-by: Kibana Machine <[email protected]>

dej611 added a commit that referenced this pull request Jan 12, 2021

[7.x] [Lens] Add specific IP and Range/Interval sorting to datatable (#…

8fd8848

…87006) (#87975) Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Add specific IP and Range/Interval sorting to datatable #87006

[Lens] Add specific IP and Range/Interval sorting to datatable #87006

dej611 commented Dec 29, 2020 •

edited

Loading

dej611 commented Dec 30, 2020

elasticmachine commented Dec 30, 2020

wylieconlon left a comment

wylieconlon Dec 30, 2020

dej611 Dec 30, 2020

wylieconlon Dec 30, 2020

wylieconlon Dec 30, 2020

dej611 commented Dec 30, 2020

tsullivan commented Dec 30, 2020

tsullivan commented Dec 31, 2020

wylieconlon commented Dec 31, 2020 •

edited

Loading

flash1293 commented Jan 4, 2021

flash1293 commented Jan 5, 2021

flash1293 left a comment

flash1293 Jan 5, 2021

dej611 Jan 11, 2021

dej611 commented Jan 11, 2021

kibanamachine commented Jan 11, 2021

	firstTable.columns.forEach((column) => {
	formatters[column.id] = formatFactory(column.meta?.params);
	});

[Lens] Add specific IP and Range/Interval sorting to datatable #87006

[Lens] Add specific IP and Range/Interval sorting to datatable #87006

Conversation

dej611 commented Dec 29, 2020 • edited Loading

Summary

How Range sorting works

How IP sorting works

Checklist

dej611 commented Dec 30, 2020

elasticmachine commented Dec 30, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Dec 30, 2020

Choose a reason for hiding this comment

dej611 Dec 30, 2020

Choose a reason for hiding this comment

wylieconlon Dec 30, 2020

Choose a reason for hiding this comment

wylieconlon Dec 30, 2020

Choose a reason for hiding this comment

dej611 commented Dec 30, 2020

tsullivan commented Dec 30, 2020

tsullivan commented Dec 31, 2020

wylieconlon commented Dec 31, 2020 • edited Loading

flash1293 commented Jan 4, 2021

flash1293 commented Jan 5, 2021

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Jan 5, 2021

Choose a reason for hiding this comment

dej611 Jan 11, 2021

Choose a reason for hiding this comment

dej611 commented Jan 11, 2021

kibanamachine commented Jan 11, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

dej611 commented Dec 29, 2020 •

edited

Loading

wylieconlon commented Dec 31, 2020 •

edited

Loading