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.

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

[TSVB] Support custom field format #101245

Merged
merged 87 commits into from
Sep 13, 2021

Conversation

DianaDerevyankina
Copy link
Contributor

@DianaDerevyankina DianaDerevyankina commented Jun 3, 2021

Closes: #97213, Closes: #32318

Summary

This PR implements support of custom field formatting for all TSVB panel types.

custom field formatting demo

Field format was added to Data formatters select as default option.

image


Cases to test for those who will review this PR:

Labels formatting

image

How to test label formatting:

  1. In the index pattern management section set field formatting for any field type that can be used for terms grouping.

    custom field formatting settings

  2. Create visualization with series grouped by terms with this field.

    image

Expect:

  • Labels shouldn't be formatted until the series is grouped by terms (i.e. in other cases simply series labels should be displayed);
  • Labels shouldn't be formatted in string index pattern mode;
  • Label formatting should be applied to all panel types;
  • Label formatting should be applied to all field types that terms grouping accepts;
  • Label formatting should work for runtime and scripted fields too;
  • Include and Exclude terms functionality should work with raw (not formatted) field names;
  • Markdown variables values should be formatted, while their keys (labels) shouldn't;
  • Fields used in annotation template should be formatted too.

Values formatting

How to test values formatting:

  1. In the index pattern management section set field formatting for any aggregatable field type.
  2. Create visualization with series based on aggregation with this field.

image

Expect:

  • Value formatting should be applied to all panel types;
  • Value formatting should be applied to all aggregatable field types;
  • Value formatting should work for runtime and scripted fields too;
  • Formatting should not be applied if aggregation is not based on any field;
  • For string index pattern Data formatters shouldn't include Default option. In case it was selected before and index pattern was switched to a string, Number option should be selected for numeric value, Custom for other value type (this could be checked in Metric with Top Hit aggregation);
  • Previous case should also be valid for series overridden index pattern;
  • For kibana index pattern mode each series should have field formatting selected by default, template should be disabled;
  • If series selected field doesn't have field formatting or string index pattern mode enabled it should have simple number formatting;
  • Other formatters should work correctly and have template enabled;
  • If aggregation returns not numeric value (e.g. string, date), Date formatters should have only Default and Custom options for kibana index pattern mode and only Custom for string index pattern. (Currently only Top Hit returns not numeric result, this could be checked in Metric);
  • For string displayed value and Custom formatter selected, Numeral.js format pattern field shouldn't be displayed, but template could be applied;
  • If series previously had some numeric formatting and was changed to the metric with a string result, it should have Default option formatting selected for kibana index pattern mode and Custom for string index pattern;
  • For the series with multiple metrics formatting should work for the visible (last) one;
  • For multiple series with different formatters Y-Axis should show simple numbers;
  • For Table, TopN and Metrics values could have Color or URL field formatters;
  • In TopN and Table Data tab Item url should have bigger priority than URL formatting;
  • In Table, TopN and Metrics color rules should have bigger priority than color formatting;
  • In case all series have the same TSVB type and template formatters or the same formatting from index pattern managements section, Y-Axis should format its values accordingly to that formatter.

Checklist

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to
identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how
they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

Add format_label response processor for series vis data and bucket key formatting to process_bucket for table vis data
@DianaDerevyankina
Copy link
Contributor Author

@elasticmachine merge upstream

@DianaDerevyankina DianaDerevyankina added Feature:TSVB TSVB (Time Series Visual Builder) release_note:enhancement labels Jun 15, 2021
@DianaDerevyankina
Copy link
Contributor Author

@elasticmachine merge upstream

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.

Code LGTM, I tested it and works fine. I think that it is a great addition for TSVB. Thank you @dziyanadzeraviankina for your effort and for your patience with it :D

I would only like @mbondyra to also do a sanity check in case I missed anything ❤️

@DianaDerevyankina DianaDerevyankina requested a review from a team as a code owner September 9, 2021 15:09
@VladLasitsa
Copy link
Contributor

LGTM, tested locally in chrome!

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Code review only 👍

Left some nitpick comments.

Comment on lines 85 to 89
const isFieldFormatter = column.formatter === DATA_FORMATTERS.DEFAULT;
const field = getMetricsField(column.metrics);
const formatter = isFieldFormatter
? createFieldFormatter(field, fieldFormatMap, 'html')
: createTickFormatter(column.formatter, column.value_template, getConfig);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: this formula gets repeated over and over in the PR. Maybe it can be isolated in a separated function that returns {isFieldFormatter, formatter}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated createFieldFormatter function, could you review that part once again, please?

src/plugins/vis_type_timeseries/server/plugin.ts Outdated Show resolved Hide resolved
@stratoula
Copy link
Contributor

stratoula commented Sep 10, 2021

I don't know if this bug is caused due to the new changes or is something that I missed but something goes really wrong here
image

Same with a number field
image

@stratoula
Copy link
Contributor

@dziyanadzeraviankina thanx, bug is fixed 🎉 Gauge changes also look great :)

@mbondyra
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks for the Sass comments. I didn't do any testing of this, just code-owner review of the Sass file.

@mbondyra
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Works as described, couldn't find anything suspicious. Approved! 👍🏼

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTimeseries 539 544 +5

Async chunks

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

id before after diff
visTypeTimeseries 1002.5KB 1017.5KB +15.0KB

Page load bundle

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

id before after diff
visTypeTimeseries 26.0KB 26.4KB +393.0B
Unknown metric groups

References to deprecated APIs

id before after diff
visTypeTimeseries 218 224 +6

History

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

cc @dziyanadzeraviankina

@DianaDerevyankina DianaDerevyankina merged commit 33cfc41 into elastic:master Sep 13, 2021
DianaDerevyankina added a commit to DianaDerevyankina/kibana that referenced this pull request Sep 13, 2021
* [TSVB] Support custom field format

Add format_label response processor for series vis data and bucket key formatting to process_bucket for table vis data

* Add ignore_field_formatting for series to support value formatting for all panel types except markdown

* Fix type issue for visData and rename getCustomFieldFormatter to createCustomFieldFormatter

* Update vis.test to cover custom field formats logic and add a migration script to set ignore_field_formatting to true for the series

* Move createCustomFieldFormatter to a separate file, make formatting respect only active metrics field name, refactor vis files and fix label formatting only for grouped by terms series

* Remove services, add getFieldFormatsService  to use it in format_label and get_table_data, replace getCustomFieldFormatter with createCustomFieldFormatter

* Update plugin.ts

* Update start for plugin.ts

* Add formatting for annotations and markdown values

* Refactor some code

* Update some naming and conditions

* Fix formatting of data type labels

* In process_bucket fix case for no getFieldFormatByName

* Add field formatting functional tests for all panel types

* Update tests to make them run correctly for firefox

* Update _tsvb_markdown test setup

* Move series ignoreFieldFormatting check to a separate file, change convertSeriesToVars signature, update migration script and refactor a bit functional tests

* Fix type check for timeseries_visualization.tsx

* Update migrations.js test expected version to 7.15

* Fix tests in _tsvb_chart.ts

* Fix merge conflict remove process_bucket.js

* Update process_bucket.test.ts

* Fix markdown labels formatting

* Add ignore_field_formatting for annotations, enhanced migration script to set that flag to true, refactor data_format_picker

* Fix migration script and add disabling for ignore component when string index pattern is used

* Add supporting URL and color formatters in tsvb table

* Fix eslint

* Remove ignore formatting component, add field formatting option to TSVB data format picker and make it default, remove migration script, update tests and refactor some files

* Fix failing tests, refactor create_field_formatter and add test to it, update some other files

* Fix series formatting for top hit when it has not numeric result

* Handle no fieldFormatMap case for table/vis.js

* Remove "Default" option form DataFormatPicker when index pattern is string, fix markdown variables issue and refactor some code

* Chore(TSVB): Replace aggregations lookup with map

* Fix types, update test expected data and remove unused translations

* Fix i18 check and useEffect in agg.tsx

* Handle aggregations field formatting case

* Fix agg_utils, vis and check_if_numeric_metric tests

* Correct typo and refactor condition in std_metric

* Fix type check

* Get rid of IFieldType

* Add URL and color formatting for topN and metric tabs, fix setting initial custom formatter and switching formatter in agg.tsx

* Update tsvb.asciidoc

* Remove link icon from Date format field help text, update click logic for top N in case of custom field format and fix CI

* Remove unused import

* Revert top N bar extra logic for click

* Refactor some code in agg.tsx

* Add URL and color formatting to Gauge

* Fix bug with terms formatting, refactor some code, update create_field_formatter

* Add comments to _gauge.scss

* Remove unnecessary await

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Uladzislau Lasitsa <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 13, 2021
…-link-to-kibana-app

* 'master' of github.com:elastic/kibana: (120 commits)
  [TSVB] Support custom field format (elastic#101245)
  [VisEditors] Add code ownership to the functional tests (elastic#111680)
  [Lens] Make Lens saved object share-capable (elastic#111403)
  [Graph] Make Graph saved object share-capable (elastic#111404)
  [Stack Monitoring] Add breadcrumb support (elastic#111850)
  Update Jira Cloud to use OAuth2.0 (elastic#111493)
  Show warning message when attempting to create an APM alert in stack management (elastic#111781)
  Skip suite blocking ES snapshot promotion (elastic#111907)
  Respect `auth_provider_hint` if session is not authenticated. (elastic#111521)
  Added in 'Responses' field in alert telemetry & updated test (elastic#111892)
  [Usage collection] refactor cloud detector collector (elastic#110439)
  Make classnames a shared dep (elastic#111636)
  Fix link to e2e tests in APM testing.md (elastic#111869)
  [Security Solution] Add host.os.name.caseless mapping and runtime field (elastic#111455)
  [APM] Removes the beta label from APM tutorial (elastic#111499) (elastic#111828)
  [RAC] [Observability] Expand Observability alerts page functional tests (elastic#111297)
  Fix extra white space on the alert table whe page size is 50 or 100 (elastic#111568)
  [Metrics UI] Add Inventory Timeline open/close state to context and URL state (elastic#111034)
  [Graph] Switch to SavedObjectClient.resolve  (elastic#109617)
  [APM] Adding lambda icon (elastic#111834)
  ...

# Conflicts:
#	x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap
DianaDerevyankina added a commit that referenced this pull request Sep 13, 2021
* [TSVB] Support custom field format

Add format_label response processor for series vis data and bucket key formatting to process_bucket for table vis data

* Add ignore_field_formatting for series to support value formatting for all panel types except markdown

* Fix type issue for visData and rename getCustomFieldFormatter to createCustomFieldFormatter

* Update vis.test to cover custom field formats logic and add a migration script to set ignore_field_formatting to true for the series

* Move createCustomFieldFormatter to a separate file, make formatting respect only active metrics field name, refactor vis files and fix label formatting only for grouped by terms series

* Remove services, add getFieldFormatsService  to use it in format_label and get_table_data, replace getCustomFieldFormatter with createCustomFieldFormatter

* Update plugin.ts

* Update start for plugin.ts

* Add formatting for annotations and markdown values

* Refactor some code

* Update some naming and conditions

* Fix formatting of data type labels

* In process_bucket fix case for no getFieldFormatByName

* Add field formatting functional tests for all panel types

* Update tests to make them run correctly for firefox

* Update _tsvb_markdown test setup

* Move series ignoreFieldFormatting check to a separate file, change convertSeriesToVars signature, update migration script and refactor a bit functional tests

* Fix type check for timeseries_visualization.tsx

* Update migrations.js test expected version to 7.15

* Fix tests in _tsvb_chart.ts

* Fix merge conflict remove process_bucket.js

* Update process_bucket.test.ts

* Fix markdown labels formatting

* Add ignore_field_formatting for annotations, enhanced migration script to set that flag to true, refactor data_format_picker

* Fix migration script and add disabling for ignore component when string index pattern is used

* Add supporting URL and color formatters in tsvb table

* Fix eslint

* Remove ignore formatting component, add field formatting option to TSVB data format picker and make it default, remove migration script, update tests and refactor some files

* Fix failing tests, refactor create_field_formatter and add test to it, update some other files

* Fix series formatting for top hit when it has not numeric result

* Handle no fieldFormatMap case for table/vis.js

* Remove "Default" option form DataFormatPicker when index pattern is string, fix markdown variables issue and refactor some code

* Chore(TSVB): Replace aggregations lookup with map

* Fix types, update test expected data and remove unused translations

* Fix i18 check and useEffect in agg.tsx

* Handle aggregations field formatting case

* Fix agg_utils, vis and check_if_numeric_metric tests

* Correct typo and refactor condition in std_metric

* Fix type check

* Get rid of IFieldType

* Add URL and color formatting for topN and metric tabs, fix setting initial custom formatter and switching formatter in agg.tsx

* Update tsvb.asciidoc

* Remove link icon from Date format field help text, update click logic for top N in case of custom field format and fix CI

* Remove unused import

* Revert top N bar extra logic for click

* Refactor some code in agg.tsx

* Add URL and color formatting to Gauge

* Fix bug with terms formatting, refactor some code, update create_field_formatter

* Add comments to _gauge.scss

* Remove unnecessary await

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Uladzislau Lasitsa <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Uladzislau Lasitsa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:TSVB TSVB (Time Series Visual Builder) release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TSVB] Support custom field format Field formatters are not applied to TSVB groups