[Lens] Custom labels for ranges #79628

flash1293 · 2020-10-06T08:52:13Z

This PR adds optional custom labels for ranges and exposes them in Lens similar to custom labels for filters.

This includes some small changes in the range AggType and range field formatter as well to map the label from the agg config into the result table and to pick up the custom label if present when formatting the value. Also, the label has to be excluded when building a filter out of a range value

lukeelmers · 2020-10-06T22:10:42Z

Stumbled across this. I know it's still in draft mode 🙈 but just wanted to add a note that whenever we implement a proper range expression type as explained in #67908, we'll also need to make sure there's a label arg on that expression function.

Currently the expression function for the range agg simply takes stringified JSON { from: 0, to: 1 }, but eventually we'll need something like range from=0 to=1 label="hi" to be provided as a subexpression for the arg.

If you want to do that here, feel free -- but no pressure. I'm mainly noting it so that we don't forget.

…ting

elasticmachine · 2020-10-07T15:10:43Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

elasticmachine · 2020-10-07T15:10:43Z

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

dej611

I tested it locally but couldn't make it work for other than the first range interval label:

dej611 · 2020-10-07T16:18:20Z

I think there's an issue with the remapping of the esaggs responses, as when I remove the first item the second ones which becomes the only range it's still without the custom range

flash1293 · 2020-10-07T17:08:26Z

Great catch, @dej611 I will check it out

…ting

flash1293 · 2020-10-08T09:57:52Z

@dej611 I didn't handle partial ranges correctly, it should be fixed now.

dej611

Did a code review. Will check the PR locally with the new changes

src/plugins/data/common/search/aggs/buckets/range_key.ts

dej611 · 2020-10-08T10:04:36Z

src/plugins/data/common/search/aggs/buckets/range_key.ts

    this.gte = bucket.from == null ? -Infinity : bucket.from;
    this.lt = bucket.to == null ? +Infinity : bucket.to;
+    this.label = allRanges ? RangeKey.findCustomLabel(this.gte, this.lt, allRanges) : undefined;


Passing bucket.from/to here can simplify the code in the findCustomLabel a bit (removing non-finite check).
They effectively equivalent, but the current version is more verbose.

Also the ternary operator can be dropped as the missing ranges would be handled by findCustomLabel

Good catch for the ternary operator, I'm not sure whether it makes it much easier because we still have to look for null/undefined.

src/plugins/data/common/search/aggs/buckets/range_key.ts

src/plugins/expressions/common/expression_types/specs/range.ts

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/ranges.tsx

…ting

flash1293 · 2020-10-08T14:01:15Z

Thanks @dej611 , pushed some changes

dej611

LGTM 👍

wylieconlon

When I use the same custom label for non-equal ranges, the result is that I get a chart with missing data. This looks like a bug to me, where I've configured two ranges called "asdf" and only one is shown:

wylieconlon · 2020-10-08T20:12:38Z

src/plugins/data/common/search/aggs/utils/get_format_with_aggs.test.ts

+    expect(format.convert({ gte: 1, lt: 20, label: 'custom' })).toBe('custom');
+    // underlying formatter is not called because custom label can be used directly
+    expect(getFormat).toHaveBeenCalledTimes(0);
+  });


Could you add a test for handling multiple overlapping ranges?

What kind of test are you expecting? The formatter only ever gets a single value passed in. Do you want to cover the behavior of the built-in cache?

I think I left this in the wrong place- I was worried about the duplicate label case, but this is the wrong place to test it.

wylieconlon · 2020-10-08T20:14:17Z

...lugins/lens/public/indexpattern_datasource/operations/definitions/ranges/advanced_editor.tsx

+          onChange={(newLabel) => {
+            const newRange = {
+              ...tempRange,
+              label: newLabel,


So this resets to the empty string to indicate that it's empty, is that going to cause problems with equality? Imagine the steps 1. Create custom label 2. Delete custom label 3. Formats correctly?

Tested, this doesn't cause any issues

flash1293 · 2020-10-08T20:37:01Z

I think the missing bar is caused by the chart only working with the formatted label so there is no difference in the two data points and only one of them is shown. We can either try to mess with chart serializers and formatters or just not allow this case, show an error in the popover and never persist such a state (which would be my preferred option)

…ting

flash1293 · 2020-10-12T12:09:57Z

@wylieconlon I tested and we have the same behavior for "Filters" - you are allowed to create two separate filters with the same custom label and only one bar will be shown. While it's not the best behavior, it's at least consistent, so I would like to keep it for the scope of this PR and open a separate bug issue to solve it consistently.

wylieconlon

@flash1293 I'm fine with the decision to allow duplicate labels in this PR, but I'll create a follow-up issue to track it as a minor annoyance.

wylieconlon · 2020-10-12T21:05:31Z

src/plugins/data/common/search/aggs/utils/get_format_with_aggs.test.ts

+    expect(format.convert({ gte: 1, lt: 20, label: 'custom' })).toBe('custom');
+    // underlying formatter is not called because custom label can be used directly
+    expect(getFormat).toHaveBeenCalledTimes(0);
+  });


I think I left this in the wrong place- I was worried about the duplicate label case, but this is the wrong place to test it.

flash1293 · 2020-10-13T08:26:29Z

@elasticmachine merge upstream

lukeelmers

Code LGTM - tested Chrome (macOS). Just had one question

lukeelmers · 2020-10-13T22:07:12Z

src/plugins/data/common/search/aggs/buckets/range.ts

@@ -67,11 +68,11 @@ export const getRangeBucketAgg = ({ getFieldFormatsStart }: RangeBucketAggDepend
        keyCaches.set(agg, keys);
      }

-      const id = RangeKey.idBucket(bucket);
+      const id = RangeKey.idBucket(bucket, agg.params.ranges);


Forgive me if this is a naive question -- but why is it necessary to change the format of the bucket id to include the label? I assume this is solving an edge case, but it wasn't completely clear to me from reading the discussion above.

I was under the impression it's necessary to not run into cached RangeKey objects with outdated labels, but I realized the cache is only used for a single request, so it's not an issue. Removed it

…ting

kibanamachine · 2020-10-14T11:09:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 051b93f

Metrics [docs]

async chunks size

id	before	after	diff
`lens`	1.0MB	1.0MB	+629.0B

page load bundle size

id	before	after	diff
`data`	1.1MB	1.1MB	+1.4KB
`expressions`	198.5KB	198.5KB	+51.0B
total			+1.4KB

History

💚 Build #81254 succeeded 4a51b81
💛 Build #81051 was flaky 9d1dfe6
💚 Build #80750 succeeded 7b2769b
💔 Build #80692 failed 1e3b9a3
💚 Build #80457 succeeded 0f24e71

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

cchaos · 2020-10-14T13:50:19Z

...lugins/lens/public/indexpattern_datasource/operations/definitions/ranges/advanced_editor.tsx

@@ -159,6 +164,25 @@ export const RangePopover = ({
          </EuiFlexItem>
        </EuiFlexGroup>
      </EuiFormRow>
+      <EuiFormRow>
+        <LabelInput


Hey @flash1293, I saw this one got merged already, but you make this input compressed since it lives in a popover and will then match the range inputs better.

flash1293 added 3 commits October 6, 2020 10:28

add custom labels for ranges

f35b205

add placeholder

a8505b3

fix range filter builder

0407327

flash1293 added release_note:enhancement v7.11.0 v8.0.0 labels Oct 6, 2020

flash1293 added 2 commits October 7, 2020 15:02

Merge remote-tracking branch 'upstream/master' into lens/range-format…

41cbf19

…ting

fix and add unit tests

0f24e71

flash1293 marked this pull request as ready for review October 7, 2020 15:10

flash1293 requested a review from a team October 7, 2020 15:10

flash1293 requested a review from a team as a code owner October 7, 2020 15:10

flash1293 added Team:AppArch Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Oct 7, 2020

dej611 requested changes Oct 7, 2020

View reviewed changes

flash1293 added 2 commits October 8, 2020 10:53

Merge remote-tracking branch 'upstream/master' into lens/range-format…

26a4334

…ting

pass label through in all cases

1e3b9a3

botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Oct 8, 2020

flash1293 requested a review from dej611 October 8, 2020 09:57

dej611 requested changes Oct 8, 2020

View reviewed changes

flash1293 added 2 commits October 8, 2020 15:48

Merge remote-tracking branch 'upstream/master' into lens/range-format…

2a4d8f0

…ting

review comments

7b2769b

dej611 approved these changes Oct 8, 2020

View reviewed changes

wylieconlon suggested changes Oct 8, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into lens/range-format…

9d1dfe6

…ting

flash1293 requested a review from wylieconlon October 12, 2020 14:44

wylieconlon approved these changes Oct 12, 2020

View reviewed changes

Merge branch 'master' into lens/range-formatting

4a51b81

lukeelmers approved these changes Oct 13, 2020

View reviewed changes

flash1293 added 2 commits October 14, 2020 11:21

Merge remote-tracking branch 'upstream/master' into lens/range-format…

5ca9b6f

…ting

remove labels from idBucket

051b93f

flash1293 merged commit 5fd7ad3 into elastic:master Oct 14, 2020

flash1293 added a commit to flash1293/kibana that referenced this pull request Oct 14, 2020

[Lens] Custom labels for ranges (elastic#79628)

a2695be

flash1293 mentioned this pull request Oct 14, 2020

[7.x] [Lens] Custom labels for ranges (#79628) #80497

Merged

timroes added the Feature:Lens label Oct 14, 2020

cchaos reviewed Oct 14, 2020

View reviewed changes

flash1293 added a commit that referenced this pull request Oct 14, 2020

[Lens] Custom labels for ranges (#79628) (#80497)

1facf63

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Custom labels for ranges #79628

[Lens] Custom labels for ranges #79628

flash1293 commented Oct 6, 2020

lukeelmers commented Oct 6, 2020

elasticmachine commented Oct 7, 2020

elasticmachine commented Oct 7, 2020

dej611 left a comment

dej611 commented Oct 7, 2020

flash1293 commented Oct 7, 2020

flash1293 commented Oct 8, 2020

dej611 left a comment

dej611 Oct 8, 2020

dej611 Oct 8, 2020

flash1293 Oct 8, 2020

flash1293 commented Oct 8, 2020

dej611 left a comment

wylieconlon left a comment

wylieconlon Oct 8, 2020

flash1293 Oct 8, 2020

wylieconlon Oct 12, 2020

wylieconlon Oct 8, 2020

flash1293 Oct 12, 2020

flash1293 commented Oct 8, 2020 •

edited

Loading

flash1293 commented Oct 12, 2020

wylieconlon left a comment

wylieconlon Oct 12, 2020

flash1293 commented Oct 13, 2020

lukeelmers left a comment

lukeelmers Oct 13, 2020

flash1293 Oct 14, 2020

kibanamachine commented Oct 14, 2020

cchaos Oct 14, 2020

[Lens] Custom labels for ranges #79628

[Lens] Custom labels for ranges #79628

Conversation

flash1293 commented Oct 6, 2020

lukeelmers commented Oct 6, 2020

elasticmachine commented Oct 7, 2020

elasticmachine commented Oct 7, 2020

dej611 left a comment

Choose a reason for hiding this comment

dej611 commented Oct 7, 2020

flash1293 commented Oct 7, 2020

flash1293 commented Oct 8, 2020

dej611 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Oct 8, 2020

dej611 left a comment

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Oct 8, 2020 • edited Loading

flash1293 commented Oct 12, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Oct 13, 2020

lukeelmers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Oct 14, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

page load bundle size

History

Choose a reason for hiding this comment

flash1293 commented Oct 8, 2020 •

edited

Loading