[Index Patterns Field Formatter] Added human readable precise formatter for duration #100540

shahzad31 · 2021-05-25T11:16:59Z

Summary

Closes #94641
Closes #11706

Added a new field formatter in index patterns for duration field to be used in observability use cases

Need to fix for elastic/uptime#327

Also added a use short suffix check box to use short suffix strings in units.

Also aded an option whether to include space between suffix /value

This is how it works on sample data

This reverts commit a590e3f

elasticmachine · 2021-05-25T11:18:57Z

Pinging @elastic/uptime (Team:uptime)

src/plugins/data/common/field_formats/converters/duration.ts

mattkime · 2021-05-25T14:44:27Z

lgtm, reaching out to @wylieconlon to see if he has any thoughts on this before I give my 👍

wylieconlon

Hi @shahzad31 thanks for taking this on! The concept is pretty much exactly what users are requesting, and here's my feedback after testing it:

Values less than 1 are bugged in this formatter, so just to take one example: with nanoseconds input, 123 nanoseconds should either display as 123 ns, 1.23 µs, or 0.000123 ms- but currently displays 0. Same with negative values. I would expect that for values less than 1, the suffix would be chosen based on the number of decimal places that are displayed. So if I choose to display 6 decimal places I would see 0.000123 ms, but if I choose 0 decimal places it should be 123 ns.
Suffix should be required for this new formatter, same as the human readable
The name "dynamic" is not very clear. I suggest renaming "human readable" -> approximate, and "dynamic" -> precise
I suggest moving this new format option to be near the top of the list, not the bottom
The value 0 should get a suffix too, as 0 ms is still a unit.
Decimal places aren't affecting the value 0, so if I have 2 decimals I want to see 0.00 ms
The suffixes you've chosen are inconsistent lengths and case, for example you have d, sec, and Yr

It would be great if the unit tests would test multiple cases per format setting, so that you can test the whole range of formats from negative, less than 1, very large, etc.

elasticmachine · 2021-05-25T20:40:19Z

Pinging @elastic/kibana-app-services (Team:AppServices)

shahzad31 · 2021-05-26T14:20:11Z

Hi @shahzad31 thanks for taking this on! The concept is pretty much exactly what users are requesting, and here's my feedback after testing it:

Values less than 1 are bugged in this formatter, so just to take one example: with nanoseconds input, 123 nanoseconds should either display as 123 ns, 1.23 µs, or 0.000123 ms- but currently displays 0. Same with negative values. I would expect that for values less than 1, the suffix would be chosen based on the number of decimal places that are displayed. So if I choose to display 6 decimal places I would see 0.000123 ms, but if I choose 0 decimal places it should be 123 ns.

i have addressed this

Suffix should be required for this new formatter, same as the human readable

fixed as well

The value 0 should get a suffix too, as 0 ms is still a unit.

Fixed

Decimal places aren't affecting the value 0, so if I have 2 decimals I want to see 0.00 ms

Fixed

The suffixes you've chosen are inconsistent lengths and case, for example you have d, sec, and Yr

I have tried improving this

It would be great if the unit tests would test multiple cases per format setting, so that you can test the whole range of formats from negative, less than 1, very large, etc.

added few more tests

shahzad31 · 2021-05-27T16:03:12Z

@shahzad31 It looks like you've introduced a new bug in the UI, which is that the form options for the precise format are disabled. Also, I would have expected that the suffix option is not shown when using the precise format.

@wylieconlon i fixed the bug, in case of human readable precise, show suffix isn't shown. Only the options to use short suffix is shown. Since that's still applicable.

wylieconlon

LGTM! This is great, users will be happy to see this.

mattkime

changes lgtm and work well

shahzad31 · 2021-05-31T07:45:32Z

@elasticmachine merge upstream

Dosant

If I understood correctly then the old Human readable should become Human-readable approximate? But it looks like that when I check out this pr old fields that were Human readable now use Human-readable precise, hence their format in apps change.
Is this behavior expected? I'd assume old fields should follow old formatting unless changed explicitly by a user?
I noticed some inconsistencies between formatting when precise vs approximate are used:
~~For example, see hours vs Hours? Would it be difficult to improve?~~ Looks like this was also the case before for other formatting options. Please ignore

Also per the images above, looks like precise returns an empty string when duration is 0 where approximate (old behavior) returns a few seconds. I would agree that the old behavior is incorrect, but also probably empty string should return something different (e.g. 0.00 seonds maybe?)? Or is this expected?

Dosant · 2021-05-31T12:29:25Z

src/plugins/data/common/field_formats/converters/duration.ts

-  text: i18n.translate('data.fieldFormats.duration.outputFormats.humanize', {
-    defaultMessage: 'Human Readable',
+  text: i18n.translate('data.fieldFormats.duration.outputFormats.humanize.precise', {
+    defaultMessage: 'Human Readable precise',


Nit copy: I think casing should be consistent here. It should either be Human Readable Precise or Human readable precise.

As I typing it out autocorrect actually suggesting me that this needs a hyphen. Then how about:

Human-readable (precise) and Human-readable (approximate) ?

I liked the suggestions. Done

…into new-field-format

shahzad31 · 2021-05-31T19:44:26Z

@Dosant so weirdly in case of default output format it doesn't set the output format value in the saved object, it goes as empty. This is why the behavior you are seeing. Will have to find some solution for this. Or otherwise i will leave the default output format as Human readable approx.

If I understood correctly then the old Human readable should become Human-readable approximate? But it looks like that when I check out this pr old fields that were Human readable now use Human-readable precise, hence their format in apps change.
Is this behavior expected? I'd assume old fields should follow old formatting unless changed explicitly by a user?

shahzad31 · 2021-06-01T07:41:47Z

@Dosant i have reverted the default format to Human-readable approximate to avoid complexities. We can tackle that in a follow up if needs be.

Dosant

Retested recent changes. LGTM 👍

…er for duration (elastic#100540) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-06-01T12:08:20Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…er for duration (#100540) (#101046) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Shahzad <shahzad.muhammad@elastic.co>

kibanamachine · 2021-06-01T14:16:25Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: a7a0d49
Storybooks Preview
Flaky suites:
- xpack-kibana-ciGroup6

Test Failures

Kibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/index_management/indices·js.apis management index management indices reload (not on Cloud) should list all the indices with the expected properties and data enrichers

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 6 times on tracked branches: https://github.com/elastic/kibana/issues/90565

[00:00:00]       │
[00:00:00]         └-: apis
[00:00:00]           └-> "before all" hook in "apis"
[00:05:45]           └-: management
[00:05:45]             └-> "before all" hook in "management"
[00:06:03]             └-: index management
[00:06:03]               └-> "before all" hook in "index management"
[00:06:03]               └-: indices
[00:06:03]                 └-> "before all" hook in "indices"
[00:06:05]                 └-: reload
[00:06:05]                   └-> "before all" hook for "should allow reloading only certain indices"
[00:06:05]                   └-> should allow reloading only certain indices
[00:06:05]                     └-> "before each" hook: global before each for "should allow reloading only certain indices"
[00:06:05]                     │ info [o.e.c.m.MetadataCreateIndexService] [kibana-ci-immutable-ubuntu-18-tests-xxl-1622549862751015791] [gsegnlzmcvmmjdwatl-1622552779684] creating index, cause [api], templates [], shards [1]/[1]
[00:06:05]                     └- ✓ pass  (67ms) "apis management index management indices reload should allow reloading only certain indices"
[00:06:05]                   └-: (not on Cloud)
[00:06:05]                     └-> "before all" hook for "should list all the indices with the expected properties and data enrichers"
[00:06:05]                     └-> should list all the indices with the expected properties and data enrichers
[00:06:05]                       └-> "before each" hook: global before each for "should list all the indices with the expected properties and data enrichers"
[00:06:05]                       └- ✖ fail: apis management index management indices reload (not on Cloud) should list all the indices with the expected properties and data enrichers
[00:06:05]                       │       Error: expected [ 'aliases',
[00:06:05]                       │   'data_stream',
[00:06:05]                       │   'documents',
[00:06:05]                       │   'health',
[00:06:05]                       │   'hidden',
[00:06:05]                       │   'ilm',
[00:06:05]                       │   'isFollowerIndex',
[00:06:05]                       │   'isFrozen',
[00:06:05]                       │   'isRollupIndex',
[00:06:05]                       │   'name',
[00:06:05]                       │   'primary',
[00:06:05]                       │   'replica',
[00:06:05]                       │   'size',
[00:06:05]                       │   'status',
[00:06:05]                       │   'uuid' ] to sort of equal [ 'aliases',
[00:06:05]                       │   'documents',
[00:06:05]                       │   'health',
[00:06:05]                       │   'hidden',
[00:06:05]                       │   'ilm',
[00:06:05]                       │   'isFollowerIndex',
[00:06:05]                       │   'isFrozen',
[00:06:05]                       │   'isRollupIndex',
[00:06:05]                       │   'name',
[00:06:05]                       │   'primary',
[00:06:05]                       │   'replica',
[00:06:05]                       │   'size',
[00:06:05]                       │   'status',
[00:06:05]                       │   'uuid' ]
[00:06:05]                       │       + expected - actual
[00:06:05]                       │ 
[00:06:05]                       │        [
[00:06:05]                       │          "aliases"
[00:06:05]                       │       -  "data_stream"
[00:06:05]                       │          "documents"
[00:06:05]                       │          "health"
[00:06:05]                       │          "hidden"
[00:06:05]                       │          "ilm"
[00:06:05]                       │       
[00:06:05]                       │       at Assertion.assert (/dev/shm/workspace/parallel/8/kibana/node_modules/@kbn/expect/expect.js:100:11)
[00:06:05]                       │       at Assertion.eql (/dev/shm/workspace/parallel/8/kibana/node_modules/@kbn/expect/expect.js:244:8)
[00:06:05]                       │       at Context.<anonymous> (test/api_integration/apis/management/index_management/indices.js:237:41)
[00:06:05]                       │       at Object.apply (/dev/shm/workspace/parallel/8/kibana/node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16)
[00:06:05]                       │ 
[00:06:05]                       │

Stack Trace

Error: expected [ 'aliases',
  'data_stream',
  'documents',
  'health',
  'hidden',
  'ilm',
  'isFollowerIndex',
  'isFrozen',
  'isRollupIndex',
  'name',
  'primary',
  'replica',
  'size',
  'status',
  'uuid' ] to sort of equal [ 'aliases',
  'documents',
  'health',
  'hidden',
  'ilm',
  'isFollowerIndex',
  'isFrozen',
  'isRollupIndex',
  'name',
  'primary',
  'replica',
  'size',
  'status',
  'uuid' ]
    at Assertion.assert (/dev/shm/workspace/parallel/8/kibana/node_modules/@kbn/expect/expect.js:100:11)
    at Assertion.eql (/dev/shm/workspace/parallel/8/kibana/node_modules/@kbn/expect/expect.js:244:8)
    at Context.<anonymous> (test/api_integration/apis/management/index_management/indices.js:237:41)
    at Object.apply (/dev/shm/workspace/parallel/8/kibana/node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16) {
  actual: '[\n' +
    '  "aliases"\n' +
    '  "data_stream"\n' +
    '  "documents"\n' +
    '  "health"\n' +
    '  "hidden"\n' +
    '  "ilm"\n' +
    '  "isFollowerIndex"\n' +
    '  "isFrozen"\n' +
    '  "isRollupIndex"\n' +
    '  "name"\n' +
    '  "primary"\n' +
    '  "replica"\n' +
    '  "size"\n' +
    '  "status"\n' +
    '  "uuid"\n' +
    ']',
  expected: '[\n' +
    '  "aliases"\n' +
    '  "documents"\n' +
    '  "health"\n' +
    '  "hidden"\n' +
    '  "ilm"\n' +
    '  "isFollowerIndex"\n' +
    '  "isFrozen"\n' +
    '  "isRollupIndex"\n' +
    '  "name"\n' +
    '  "primary"\n' +
    '  "replica"\n' +
    '  "size"\n' +
    '  "status"\n' +
    '  "uuid"\n' +
    ']',
  showDiff: true
}

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`data`	3223	3224	+1

Page load bundle

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

id	before	after	diff
`data`	817.4KB	820.8KB	+3.4KB
`indexPatternFieldEditor`	81.8KB	83.1KB	+1.3KB
total			+4.7KB

Unknown metric groups

API count

id	before	after	diff
`data`	3768	3769	+1

History

💚 Build #128248 succeeded a7a0d49
💚 Build #128131 succeeded dbfadda
💚 Build #127708 succeeded 1cef50e
💚 Build #127444 succeeded 55d0511
💚 Build #127395 succeeded cec6521

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

cc @shahzad31

shahzad31 added 8 commits May 18, 2021 15:16

added dynamic formatter for duration

863981b

Merge branch 'master' into new-field-format

8a67330

update test

f345ff5

add test

0b2094a

Merge branch 'master' into new-field-format

1406be4

wip

a590e3f

Merge branch 'master' into new-field-format

85aa71a

Revert "wip"

bdebc00

This reverts commit a590e3f

shahzad31 changed the title ~~added dynamic formatter for duration~~ [Index Patterns Field Formatter] Added dynamic formatter for duration May 25, 2021

shahzad31 marked this pull request as ready for review May 25, 2021 11:18

shahzad31 requested a review from a team as a code owner May 25, 2021 11:18

shahzad31 self-assigned this May 25, 2021

shahzad31 added release_note:enhancement v7.14.0 v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels May 25, 2021

shahzad31 added 2 commits May 25, 2021 13:20

Merge branch 'master' into new-field-format

3ea640a

update type

d006e99

shahzad31 requested a review from formgeist May 25, 2021 12:30

formgeist approved these changes May 25, 2021

View reviewed changes

src/plugins/data/common/field_formats/converters/duration.ts Outdated Show resolved Hide resolved

wylieconlon suggested changes May 25, 2021

View reviewed changes

wylieconlon added Feature:FieldFormatters Team:AppServices labels May 25, 2021

Merge branch 'master' into new-field-format

f9bdea5

wylieconlon mentioned this pull request May 25, 2021

Field format : add suffix option #8984

Closed

Pr feedback plus option to include space

cec6521

shahzad31 added 3 commits May 27, 2021 14:30

Merge branch 'master' into new-field-format

93b842a

wip

839c018

fix issues with disable

1cef50e

mattkime requested a review from Dosant May 27, 2021 16:11

shahzad31 requested a review from wylieconlon May 28, 2021 15:26

wylieconlon approved these changes May 28, 2021

View reviewed changes

mattkime approved these changes May 28, 2021

View reviewed changes

Merge branch 'master' into new-field-format

dbfadda

Dosant reviewed May 31, 2021

View reviewed changes

This was referenced May 31, 2021

[FieldFormatters] Human-readable duration shows "a few seconds" when duration is "0" #101000

Closed

[FieldFormatters] Human-readable duration inconsistent unit casing #101002

Closed

shahzad31 added 2 commits May 31, 2021 21:09

Merge branch 'master' into new-field-format

e5203fe

Merge branch 'new-field-format' of https://github.com/shahzad31/kibana …

4fed2ef

…into new-field-format

shahzad31 added 2 commits May 31, 2021 21:57

revert default out param

6727451

return value in case of 0

a7a0d49

shahzad31 requested a review from Dosant June 1, 2021 07:41

Dosant approved these changes Jun 1, 2021

View reviewed changes

shahzad31 added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 1, 2021

shahzad31 merged commit 4aa3920 into elastic:master Jun 1, 2021

shahzad31 deleted the new-field-format branch June 1, 2021 12:05

kibanamachine mentioned this pull request Jun 1, 2021

[7.x] [Index Patterns Field Formatter] Added human readable precise formatter for duration (#100540) #101046

Merged

shahzad31 mentioned this pull request Jun 8, 2021

[Exploratory View] Use human readable formats #101520

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Index Patterns Field Formatter] Added human readable precise formatter for duration #100540

[Index Patterns Field Formatter] Added human readable precise formatter for duration #100540

shahzad31 commented May 25, 2021 •

edited

Loading

elasticmachine commented May 25, 2021

mattkime commented May 25, 2021

wylieconlon left a comment

elasticmachine commented May 25, 2021

shahzad31 commented May 26, 2021

shahzad31 commented May 27, 2021

wylieconlon left a comment

mattkime left a comment

shahzad31 commented May 31, 2021

Dosant left a comment •

edited

Loading

Dosant May 31, 2021

shahzad31 May 31, 2021

shahzad31 commented May 31, 2021

shahzad31 commented Jun 1, 2021

Dosant left a comment

kibanamachine commented Jun 1, 2021

kibanamachine commented Jun 1, 2021

Standard Out

Stack Trace

API count

[Index Patterns Field Formatter] Added human readable precise formatter for duration #100540

[Index Patterns Field Formatter] Added human readable precise formatter for duration #100540

Conversation

shahzad31 commented May 25, 2021 • edited Loading

Summary

elasticmachine commented May 25, 2021

mattkime commented May 25, 2021

wylieconlon left a comment

Choose a reason for hiding this comment

elasticmachine commented May 25, 2021

shahzad31 commented May 26, 2021

shahzad31 commented May 27, 2021

wylieconlon left a comment

Choose a reason for hiding this comment

mattkime left a comment

Choose a reason for hiding this comment

shahzad31 commented May 31, 2021

Dosant left a comment • edited Loading

Choose a reason for hiding this comment

Dosant May 31, 2021

Choose a reason for hiding this comment

shahzad31 May 31, 2021

Choose a reason for hiding this comment

shahzad31 commented May 31, 2021

shahzad31 commented Jun 1, 2021

Dosant left a comment

Choose a reason for hiding this comment

kibanamachine commented Jun 1, 2021

💚 Backport successful

kibanamachine commented Jun 1, 2021

💛 Build succeeded, but was flaky

Test Failures

Standard Out

Stack Trace

Metrics [docs]

Public APIs missing comments

Page load bundle

API count

History

shahzad31 commented May 25, 2021 •

edited

Loading

Dosant left a comment •

edited

Loading