[Lens] Normalize values by time unit by flash1293 · Pull Request #83904 · elastic/kibana

flash1293 · 2020-11-20T11:10:57Z

This PR puts the lens_time_scale expression function and the suffix formatter to use and allows to specify the time unit to scale a value by (if there's a date histogram available)

app-services changes

The changes in the expression plugin are minimal - it's just adding an option to allow overwriting an existing column with the output value for the buildResultColumn helper.

flash1293 · 2020-11-23T10:05:13Z

@elasticmachine merge upstream

kibanamachine · 2020-11-23T10:05:15Z

merge conflict between base and head

flash1293 · 2020-11-23T13:33:40Z

@elasticmachine merge upstream

elasticmachine · 2020-11-23T14:04:04Z

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

lukeelmers

Small nits, but nothing critical -- app services code LGTM. 👍

lukeelmers · 2020-11-23T19:32:49Z

src/plugins/expressions/common/expression_functions/series_calculation_helpers.ts

  inputColumnId: string,
-  outputColumnName: string | undefined
+  outputColumnName: string | undefined,
+  options: { allowColumnOverride: boolean } = { allowColumnOverride: false }


nit: Could we add the new option to the jsdoc so there's a brief explanation in the comments?

lukeelmers · 2020-11-23T19:35:20Z

src/plugins/expressions/common/expression_functions/series_calculation_helpers.ts

 ) {
-  if (input.columns.some((column) => column.id === outputColumnId)) {
+  if (
+    !options.allowColumnOverride &&


naming nit: I think allowColumnOverwrite would be a bit more intuitive than "Override", but I don't feel strongly about this.

wylieconlon

This is overall working very well! Left a lot of small code and naming comments, but I think it's enough to require a second round. There's also these questions that I don't think are required, but that might affect the layout of the code:

What happens if in the future we want to support the suffix formatter as an extra formatter, without time scaling? For example, metrics are commonly stored as "per second" values.

What happens if in the future we want to support time scaling without a suffix?

What happens if in the future we want to support time scaling without the date histogram?

wylieconlon · 2020-11-23T21:57:49Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/time_scaling.tsx

+          <EuiText size="s">
+            <EuiLink
+              data-test-subj="indexPattern-time-scaling-enable"
+              color="text"


Can we use the default EuiLink styling instead of removing the affordances of the color and underline?

I followed the mockups here:

IMHO this makes sense because of the similarity to the official context menu component:

I won't block this any more, but I do think this is a confusing design because it lacks the affordances of the EUI examples. The main reason I think this is problematic is that it's using a menu-oriented design, but only has one item.

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/time_scaling.tsx

wylieconlon · 2020-11-23T22:20:34Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/time_scaling.tsx

+      fullWidth
+      label={i18n.translate('xpack.lens.indexPattern.timeScale.label', {
+        defaultMessage: 'Normalize by unit',
+      })}


I'd like to add a tooltip to this form to explain the most common questions, especially the "why":

Normalized values are displayed as a rate, calculated from the date histogram interval. Values can scale down from a larger interval or to scale up a smaller interval. Partial intervals are allowed.

wylieconlon · 2020-11-23T22:23:35Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/time_scaling.tsx

+                      ...state.layers[layerId].columns,
+                      [columnId]: {
+                        ...selectedColumn,
+                        timeScale: e.target.value as TimeScaleUnit,


I would prefer that we update the default label every time the time scale is changed. I found it pretty confusing when the axis labels were showing "timestamp per 30 minutes" but my data is "Count of records" instead of "Count of records per second"

Good Idea, I will integrate this.

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/time_scaling.tsx

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx

wylieconlon · 2020-11-23T22:32:24Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/time_scaling.tsx

+import { TimeScaleUnit } from '../time_scale';
+import { IndexPatternPrivateState } from '../types';
+
+const DEFAULT_TIME_SCALE = 'm' as const;


Why is this the default, and not per-second? I ask because we can pretty much guarantee that per-second is smaller than the interval they have, so it's unlikely to scale up. I think that's a benefit.

Took that from the mocks as well, but I see your point and I think per second is a better default

wylieconlon · 2020-11-23T22:36:31Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/column_types.ts

  // Private
  operationType: string;
  customLabel?: boolean;
+  timeScale?: TimeScaleUnit;


Why did you add this to the top-level IndexPatternColumn type instead of to params? We have more uses of column.params, including when building the expression output.

I thought it would be a better place to clarify it's not managed by the individual operations, but it makes more sense to put it there anyway. I will adjust the PR.

@wylieconlon Looking into this I would like to keep it as it is as the types for params are pretty complex right now and I would have to overwrite them in weird ways in a bunch of places to make it work. We should clean that up but I don't think we have to do it on this PR. Ideally I would like to keep the frame managed pieces to the top level, with params being reserved for the individual operations. This makes the types much simpler.

wylieconlon · 2020-11-23T22:37:19Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts

  data: DataPublicPluginStart;
 }

+export type TimeScalingMode = 'disabled' | 'mandatory' | 'optional';


It looks like mandatory time scaling isn't handled in this PR: nothing is different in the UI if it's mandatory vs optional. Intentional?

The "remove" button is not shown if it's set to mandatory (implicitly checking the absence of mandatory by checking for optional in a branch of the code that ruled out undefined and disabled already)

wylieconlon · 2020-11-23T22:38:20Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/time_scaling.tsx

+                        ...state.layers[layerId].columns,
+                        [columnId]: {
+                          ...selectedColumn,
+                          timeScale: undefined,


If timeScale were part of params, you'd be allowed to use the updateColumnParam helper from layer_helpers. Would be simpler than writing all this!

Unfortunately this idea conflicts with my idea of using a new updateLayer function instead of setState- we can only pick one of these for simplification.

I think updateLayer makes sense here because we anyway have to adjust the label as well based on your other comment

flash1293 · 2020-11-24T13:22:39Z

Thanks for your comments @wylieconlon , I'm currently working through them. I noticed the transition logic doesn't work at the moment, I'm adjusting this (lot's of edge cases in there!).

About your questions:

What happens if in the future we want to support the suffix formatter as an extra formatter, without time scaling? For example, metrics are commonly stored as "per second" values.

It would be a separate thing to let the user configure next to the regular format - we already have the "parentFormat" field in there, it would be about making this configurable by the user. The time scaling code would have to check for that to make sure it doesn't get applied twice.

What happens if in the future we want to support time scaling without a suffix?

Same as above - "parentFormat" would be explicitly set, and the time scaling expression logic checks for that and adjusts

What happens if in the future we want to support time scaling without the date histogram?

I think the logic for that can be encapsulated in the time_scale expression function and the expression building code. I don't think we have to touch anything else (besides removing the check in the UI to hide the time scaling component if there's not date histogram)

flash1293 · 2020-11-24T14:42:18Z

@wylieconlon OK, should be ready to review again. I implemented most of your comments and left replies in cases I didn't. Also I made sure the time scale property is transitioned correctly in the places necessary right now - once we introduce transitions for calculations we have to make sure it works correctly there as well.

mbondyra

Tested on Safari, code looks good to me 🆗

wylieconlon

I think there are two comments that should be addressed before merging, but I'm approving because I don't think I need to give it another pass.

Tested in FF and the functionality is looking good!

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/time_scaling.tsx

wylieconlon · 2020-11-25T17:27:07Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/time_scaling.tsx

+          <EuiText size="s">
+            <EuiLink
+              data-test-subj="indexPattern-time-scaling-enable"
+              color="text"


I won't block this any more, but I do think this is a confusing design because it lacks the affordances of the EUI examples. The main reason I think this is problematic is that it's using a menu-oriented design, but only has one item.

wylieconlon · 2020-11-25T17:31:50Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts

+  if (previousTimeScale) {
+    const suffixPosition = oldLabel.lastIndexOf(` ${unitSuffixesLong[previousTimeScale]}`);
+    if (suffixPosition !== -1) {
+      cleanedLabel = oldLabel.substring(0, suffixPosition);


I think you should change this implementation before merging, because I think that the reference editor work will cause the time scaling suffixes to get removed accidentally. The reference logic is calling getDefaultLabel when modifying columns, so I think the suffix adding needs to move into the operation definitions. A nice benefit of this is that you won't need to do any string manipulation here.

Can you clarify a little how you expect it to work? The label change would happen in the getDefaultLabel or buildColumn function of the operations supporting time scaling?

flash1293 · 2020-11-26T10:28:36Z

Thanks for the review @wylieconlon , I moved the logic of adjusting the label and carrying over the time scale param into the individual operations. While doing that I made sure the calculations are covered as well. I also fixed an additional case I stumbled over - if the date histogram gets removed, the label has to be adjusted as well. Luckily we already had the onOtherColumnsChanged hook in individual operations.

In a very recent bug fix of mine (#84121) I wrote the following:

Removing the useEffect updating on root state changes fixes this behavior, but introduces a theoretical bug of not reflecting a change somewhere else. However I reviewed the places these inputs are used and we don't have such a case yet. So this is fine for now to fix the bug at hand and we can come back to solve it better once we refactor the state mangement.

Well, this time is now, because if the user interacts with the time scaling UI, the label gets updated from the outside, but it would not be shown because the internal state is cached already. I think I found a better solution, implemented on this PR - in the debounced input, I'm keeping track of whether a debounced change caused by the user typing is pending right now to be pushed up the the root state. If that's the case, incoming value updates are discarded (to not get into the way of the typing user). If there are no in-flight changes caused by typing, the value from the root state is copied into the local state. This should fix both cases - keeping the input snappy and making sure the state is always consistent. @mbondyra could you take a look at that part?

flash1293 · 2020-11-26T10:44:54Z

I also changed the wording a little because "Normalize by time unit" is a little too long for the tooltip icon to not wrap into a new line:

I think we can improve wording, but I would like to keep iterating on this off this PR and do a whole pass over the UI by the end of the current development cycle.

mbondyra · 2020-11-30T10:58:57Z

Reapproved, I'm not totally convinced to 'normalize by unit' copy, but we can iterate on that. Works fine and code looks good.

…bana into add-metadata-to-node-details * 'add-metadata-to-node-details' of github.com:phillipb/kibana: [APM] ML anomaly detection integration: Displaying anomaly job results in the Transaction duration chart is not as intended (elastic#84415) Support for painless language autocomplete within monaco (elastic#80577) [Lens] Time scale ui (elastic#83904) removing beta callouts (elastic#84510) [Lens] (Accessibility) add aria-label to chart type icon (elastic#84493) Trusted Apps signer API. (elastic#83661) increase stdout max listeners for legacy logging (elastic#84497) [APM] Service overview: Add throughput chart (elastic#84439) [Discover] Unskip main functional tests (elastic#84300) Uptime overview overhaul (elastic#83406) [APM] Adjust time formats based on the difference between start and end (elastic#84470) [ML] Renaming saved object repair to sync (elastic#84311) [UsageCollection] Remove `formatBulkUpload` and other unused APIs (elastic#84313) [Visualizations] Adds visConfig.title and uiState to build pipeline function (elastic#84456) [Elasticsearch Migration] Update docs re UsageCollection (elastic#84322) TSVB field list performance issue on using annotations (elastic#84407) [Security Solution] Exceptions Cypress tests (elastic#81759) [ML] Fix spaces job ID check (elastic#84404) [Security Solution][Detections] Handle dupes when processing threshold rules (elastic#83062)

* master: (25 commits) [Alerting] fixes buggy default message behaviour (elastic#84202) [Graph] Use new ES client and change license API (elastic#84398) [DOCS] Adds redirect to known plugins page (elastic#84001) Update IndexPatternSelect to get fields from indexPatternService instead of savedObject attributes (elastic#84376) Adding timestamps to created events so the sorting is stable (elastic#84515) [DOCS] Redirects for drilldown links (elastic#83846) [Fleet] Support for showing an Integration Detail Custom (UI Extension) tab (elastic#83805) [Enterprise Search] Migrate shared Schema components (elastic#84381) [Discover] Unskip date_nanos and shard links functional tests (elastic#82878) [APM] ML anomaly detection integration: Displaying anomaly job results in the Transaction duration chart is not as intended (elastic#84415) Support for painless language autocomplete within monaco (elastic#80577) [Lens] Time scale ui (elastic#83904) removing beta callouts (elastic#84510) [Lens] (Accessibility) add aria-label to chart type icon (elastic#84493) Trusted Apps signer API. (elastic#83661) increase stdout max listeners for legacy logging (elastic#84497) [APM] Service overview: Add throughput chart (elastic#84439) [Discover] Unskip main functional tests (elastic#84300) Uptime overview overhaul (elastic#83406) [APM] Adjust time formats based on the difference between start and end (elastic#84470) ...

wylieconlon · 2020-12-01T21:58:55Z

@flash1293 Do you think the title of this PR should be updated so that it fits in the release notes?

kibanamachine · 2020-12-02T10:11:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6ab3f08

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	445	447	+2

Async chunks

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

id	before	after	diff
`lens`	953.8KB	966.7KB	+13.0KB

Page load bundle

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

id	before	after	diff
`expressions`	182.4KB	182.5KB	+68.0B

History

💚 Build #90593 succeeded 6ab3f08
💚 Build #90403 succeeded 758311a
💚 Build #90398 succeeded 07fddda
💚 Build #90392 succeeded bc5d789
💚 Build #89962 succeeded 69ecfbd

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

wylieconlon and others added 4 commits November 17, 2020 18:55

[Lens] Implement types for reference-based operations

eb4b573

start working on time scaling UI

a83c727

Merge remote-tracking branch 'upstream/master' into lens/time-scale-ui-2

7e53024

add tests and styling

de19d24

flash1293 added release_note:enhancement v8.0.0 Feature:Lens v7.11.0 labels Nov 20, 2020

flash1293 added 2 commits November 23, 2020 12:13

Merge remote-tracking branch 'upstream/master' into lens/time-scale-ui-2

220052d

remove integration logic

5499ead

Merge branch 'master' into lens/time-scale-ui-2

90f21a3

flash1293 marked this pull request as ready for review November 23, 2020 14:03

flash1293 requested a review from a team November 23, 2020 14:03

flash1293 requested a review from a team as a code owner November 23, 2020 14:03

flash1293 added the Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// label Nov 23, 2020

lukeelmers approved these changes Nov 23, 2020

View reviewed changes

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

wylieconlon suggested changes Nov 23, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into lens/time-scale-ui-2

c8a9530

review comments

69ecfbd

flash1293 requested a review from wylieconlon November 24, 2020 17:45

mbondyra approved these changes Nov 25, 2020

View reviewed changes

wylieconlon approved these changes Nov 25, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into lens/time-scale-ui-2

7f39f59

flash1293 added 3 commits November 26, 2020 09:57

fix stuff

20db54f

Merge remote-tracking branch 'upstream/master' into lens/time-scale-ui-2

b7bb2bc

add tests and fix label input

bc5d789

flash1293 added 2 commits November 26, 2020 11:37

change wording

07fddda

adjust wording

758311a

flash1293 added 2 commits November 30, 2020 07:39

Merge remote-tracking branch 'upstream/master' into lens/time-scale-ui-2

41200dd

move tests

6ab3f08

flash1293 merged commit 6828859 into elastic:master Nov 30, 2020

flash1293 mentioned this pull request Nov 30, 2020

[7.x] [Lens] Normalize values by time unit (#83904) #84520

Merged

flash1293 added a commit to flash1293/kibana that referenced this pull request Nov 30, 2020

[Lens] Time scale ui (elastic#83904)

522369c

flash1293 added a commit that referenced this pull request Nov 30, 2020

[Lens] Time scale ui (#83904) (#84520)

91a2b68

flash1293 changed the title ~~[Lens] Time scale ui~~ [Lens] Normalize values by time unit Dec 2, 2020

Conversation

flash1293 commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

app-services changes

Uh oh!

flash1293 commented Nov 23, 2020

Uh oh!

kibanamachine commented Nov 23, 2020

Uh oh!

flash1293 commented Nov 23, 2020

Uh oh!

elasticmachine commented Nov 23, 2020

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wylieconlon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Nov 24, 2020

Uh oh!

mbondyra left a comment

Choose a reason for hiding this comment

Uh oh!

wylieconlon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

flash1293 commented Nov 20, 2020 •

edited

Loading

flash1293 Nov 24, 2020 •

edited

Loading

flash1293 Nov 24, 2020 •

edited

Loading

flash1293 commented Nov 24, 2020 •

edited

Loading

flash1293 commented Nov 26, 2020 •

edited

Loading