[Rollups] Rollup support in Kibana, phase 1 #21117

cjcenizal · 2018-07-23T23:20:07Z

Meta issue: #20004

Overview

This PR introduces rollup support to Kibana. Specifically, the ability to:

View/Manage/Create rollup jobs
Create a rollup index pattern
Create rollup visualizations
Add rollup visualizations to dashboards
View raw rollup documents in Discover

Testing plan

Please refer to this document for detailed information and testing strategy for each aspect of rollup support: Rollups testing plan

Rollup job management

cjcenizal · 2018-09-17T19:57:41Z

@maryia-lapata I've localized the copy in x-pack/plugins/rollup/public/crud_app. When you have time could you please review and let me know if I missed any copy or made any mistakes? Thank you!

maryia-lapata · 2018-09-19T20:24:15Z

...ions/indices/create_index_pattern_wizard/components/step_index_pattern/step_index_pattern.js

@@ -245,12 +252,15 @@ export class StepIndexPatternComponent extends Component {
      const errorMessage = intl.formatMessage(
        {
          id: 'kbn.management.createIndexPattern.step.invalidCharactersErrorMessage',
-          defaultMessage: 'An index pattern cannot contain spaces or the characters: {characterList}'
+          defaultMessage: 'A ${indexPatternName} cannot contain spaces or the characters: {characterList}'


Please remove $ sign. Placeholders should be presented just in curly braces.

maryia-lapata · 2018-09-19T20:34:01Z

...anagement/sections/indices/edit_index_pattern/indexed_fields_table/components/table/table.js

-    const label = intl.formatMessage({
+
+    const infoLabel = intl.formatMessage({
+      id: 'kbn.management.editIndexPattern.fields.table.additionalInfo',


Please add a description of label type to the id. Since it's a label for aria-label attribute, id should end with AriaLabel.
kbn.management.editIndexPattern.fields.table.additionalInfoAriaLabel

maryia-lapata · 2018-09-19T20:35:55Z

...anagement/sections/indices/edit_index_pattern/indexed_fields_table/components/table/table.js

+      id: 'kbn.management.editIndexPattern.fields.table.additionalInfo',
+      defaultMessage: 'Additional field information'
+    });
+    const timeLabel = intl.formatMessage({
      id: 'kbn.management.editIndexPattern.fields.table.primaryTimeAria',


nit: Please update the id according to Guideline: kbn.management.editIndexPattern.fields.table.primaryTimeAriaLabel

maryia-lapata · 2018-09-19T20:38:07Z

src/core_plugins/kibana/public/management/sections/indices/index.html

-        </a>
-      </li>
-    </ul>
+    <div id="indexPatternListReact" role="region" aria-label="Index patterns"></div>


I think, this aria-label attribute should be translated as well.

maryia-lapata · 2018-09-19T20:42:33Z

...ins/kibana/public/management/sections/indices/index_pattern_list/components/header/header.js

+  indexPatternCreationOptions
+}) => (
+  <CreateButton options={indexPatternCreationOptions}>
+    Create index pattern


I suppose that Create index pattern should be translated as well.

maryia-lapata · 2018-09-19T21:08:21Z

...ic/crud_app/sections/components/job_action_menu/confirm_delete_modal/confirm_delete_modal.js

+  renderJobs() {
+    const { jobs } = this.props;
+    const jobItems = jobs.map(({ id, status }) => {
+      const statusText = status === 'started' ? ' (started)' : null;


(started) should be translated.

maryia-lapata · 2018-09-19T21:13:53Z

x-pack/plugins/rollup/public/crud_app/sections/components/job_action_menu/job_action_menu.js

+      <EuiButton
+        data-test-subj="jobActionMenuButton"
+        iconSide={iconSide}
+        aria-label={`${entity} options`}


Please translate aria-label attribute.

maryia-lapata · 2018-09-19T21:16:54Z

x-pack/plugins/rollup/public/crud_app/sections/components/job_details/tabs/tab_histogram.js

+
+const columns = [{
+  field: 'name',
+  name: 'Field',


Please translate this label.

maryia-lapata · 2018-09-19T21:18:10Z

x-pack/plugins/rollup/public/crud_app/sections/components/job_details/tabs/tab_metrics.js

+
+const columns = [{
+  field: 'name',
+  name: 'Field',


Please translate this field.

maryia-lapata · 2018-09-19T21:18:23Z

x-pack/plugins/rollup/public/crud_app/sections/components/job_details/tabs/tab_metrics.js

+  truncateText: true,
+  sortable: true,
+}, {
+  name: 'Types',


and this one

maryia-lapata

Added some more comments

maryia-lapata · 2018-09-25T09:13:19Z

src/core_plugins/kibana/public/management/sections/objects/lib/resolve_saved_objects.js

@@ -36,22 +36,36 @@ function addJsonFieldToIndexPattern(target, sourceString, fieldName, indexName)
    try {
      target[fieldName] = JSON.parse(sourceString);
    } catch (error) {
-      throw new Error(`Error encountered parsing ${fieldName} for index pattern ${indexName}: ${error.message}`);
+      throw new Error(
+        `Error encountered parsing ${fieldName} for index pattern ${indexName}: ${error.message}`


if a user can see this message, it should be translated as well.

maryia-lapata · 2018-09-25T10:17:26Z

x-pack/plugins/rollup/public/crud_app/sections/components/job_details/tabs/tab_json.js

+  );
+};
+
+export const TabJson = injectI18n(TabJsonUi);


There are no translated labels, so it's no reason to wrap the component with injectI18n

maryia-lapata · 2018-09-25T10:20:31Z

x-pack/plugins/rollup/public/crud_app/sections/components/job_details/tabs/tab_summary.js

+            <EuiFlexItem>
+              <EuiDescriptionListTitle>
+                <FormattedMessage
+                  id="xpack.rollupJobs.jobDetails.tabSummary.itemDocumentsProcessed.label"


nit: To be consistent with other code, please follow to naming convention described in Guideline. Please use camelcase for the message type: xpack.rollupJobs.jobDetails.tabSummary.itemDocumentsProcessedLabel

maryia-lapata · 2018-09-25T10:21:17Z

x-pack/plugins/rollup/public/crud_app/sections/components/job_details/tabs/tab_summary.js

+            <EuiFlexItem>
+              <EuiDescriptionListTitle>
+                <FormattedMessage
+                  id="xpack.rollupJobs.jobDetails.tabSummary.itemPagesProcessed.label"


nit: xpack.rollupJobs.jobDetails.tabSummary.itemPagesProcessedLabel

yaronp68 · 2018-09-25T11:11:30Z

@cjcenizal
In the last phase consider adding a checkbox / button to allow all stats to be ticked per metric row. currently user need to tick each stat

yaronp68 · 2018-09-25T11:15:45Z

@cjcenizal in the last phase consider adding a user friendly version to the cron pattern. the field should be named like in the wizard: Schedule (or we can use frequency)

elasticmachine · 2018-10-23T04:18:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

I went digging around in some of the places that @cjcenizal pointed out and found somethings that I think should be addressed, but nothing earth shattering.

I'm a little shocked to not find a single test in the rollup UI. Thankfully there are tests for the server libs, but there should absolutely be more unit tests in the UI and some functional tests to verify baseline functionality end-to-end.

I know we're trying to get this in for 6.5, and since most of this logic is out of the critical path it shouldn't be too problematic, I'm fine if y'all merge this and work to address testing/feedback in follow up PRs, but I'm going to have to trust that you follow through with that as I won't be able to verify everything is taken care of.

spalger · 2018-10-24T03:11:29Z

x-pack/plugins/rollup/public/crud_app/services/http_provider.js

+let _http;
+
+export function setHttp(http) {
+  _http = http;


Out of curiosity, why not use kfetch?

I think we're blocked by #22594 (interceptor parity between kfetch and $http). We originally used kfetch and then switched to $http for this reason.

spalger · 2018-10-24T03:12:23Z

x-pack/plugins/rollup/public/crud_app/services/humanized_numbers.js

+};
+
+export function getOrdinalValue(number) {
+  // TODO: This is breaking reporting pdf generation. Possibly due to phantom not setting locale,


Is there an issue for this?

Created #24770

spalger · 2018-10-24T03:14:52Z

x-pack/plugins/rollup/public/crud_app/services/jobs.js

+
+function removeEmptyValues(object) {
+  Object.keys(object).forEach(key => {
+    if (object[key] == null || object[key].trim() === '') {


I think relying on the values in object to be either undefined, null, or a string is slightly risky. String(object[key]).trim() === '' would probably be a bit safer.

Could you clarify what you meant by "safer"? Are you concerned about object being of type Object or number?

spalger · 2018-10-24T03:16:43Z

x-pack/plugins/rollup/public/crud_app/services/query_params.js

+  }
+
+  const extractedQueryParams = {};
+  const queryParamPairs = queryString.split('?')[1].split('&').map(paramString => paramString.split('='));


Can we please use require('query_string').parse() instead of reimplementing query string parsing without tests?

spalger · 2018-10-24T03:24:04Z

x-pack/plugins/rollup/public/crud_app/store/actions/change_job_status.js

+  try {
+    await sendStartJobsRequest(jobIds);
+  } catch (error) {
+    return toastNotifications.addDanger(error.data.message);


I'm not confident that errors will always have a data.message property, we should probably use a helper of some sort that uses that when available, and defaults to something like error.stack when it isn't...

x-pack/plugins/rollup/public/crud_app/store/actions/create_job.js

+    [ newJob ] = await Promise.all([
+      sendCreateJobRequest(serializeJob(jobConfig)),
+      // Wait at least half a second to avoid a weird flicker of the saving feedback.
+      new Promise(resolve => setTimeout(resolve, 500)),


spalger · 2018-10-24T03:27:36Z

x-pack/plugins/rollup/public/crud_app/store/actions/create_job.js

+      new Promise(resolve => setTimeout(resolve, 500)),
+    ]);
+  } catch (error) {
+    const { statusCode, data } = error;


There are lots of situations where error here is not coming from the API, so I think we should handle those errors differently rather than assuming every error will be an HTTP response.

spalger · 2018-10-24T03:29:08Z

x-pack/plugins/rollup/public/crud_app/store/actions/create_job.js

+    const { statusCode, data } = error;
+
+    // Some errors have statusCode directly available but some are under a data property.
+    switch (statusCode || data.statusCode) {


I think this makes more sense as a if ((statusCode || data.statusCode) === 409) block with an early return, followed by the default case and an early return, rather than a switch statement with a floating return after it.

spalger · 2018-10-24T03:40:33Z

x-pack/plugins/rollup/public/crud_app/sections/job_list/job_list.js

+    const { isLoading, jobs, jobLoadError } = this.props;
+
+    let content;
+    if (jobLoadError && jobLoadError.status === 403) {


If we have a jobLoadError and it's not because of a permission error, we should probably do something besides load an empty screen, right?

spalger · 2018-10-24T03:48:12Z

src/ui/public/utils/parse_es_interval/parse_es_interval.ts

 import dateMath from '@kbn/datemath';

+import { InvalidEsCalendarIntervalError } from './invalid_es_calendar_interval_error';
+import { InvalidEsIntervalFormatError } from './invalid_es_interval_format_error';
+
 const ES_INTERVAL_STRING_REGEX = new RegExp(
  '^([1-9][0-9]*)\\s*(' + dateMath.units.join('|') + ')$'
 );


According to https://github.com/elastic/kibana/blob/master/packages/kbn-datemath/src/index.js#L25 m is a mixed type unit, meaning a single minute is a "calendar" unit type, right? Does that mean that the datemath module is wrong or this module is wrong?

Also why is this not part of the datemath module? Feels like it's pretty closely related.

I believe we either hashed this out or it is being addressed in #24671? Let me know if this is actually a separate issue.

elasticmachine · 2018-10-24T08:41:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…to allow rollups to avoid normalizing ES interval units (#24428)

* add rollups link to Kibana home * copy change * copy edit * Making rollup jobs empty message match home page copy * copy edit * copy edit

bmcconaghy

Commenting out works to prevent creation of rollup index pattern. Code LGTM.

elasticmachine · 2018-10-24T23:31:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-25T00:38:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Enabled: - View/Manage/Create rollup jobs Disabled: - Create a rollup index pattern - Create rollup visualizations - Add rollup visualizations to dashboards - View raw rollup documents in Discover

cjcenizal added WIP Work in progress :Management labels Jul 23, 2018

cjcenizal assigned cjcenizal and jen-huang Jul 23, 2018

jen-huang force-pushed the feature/rollups branch from 3be85a2 to 189d35c Compare July 24, 2018 07:03

jen-huang force-pushed the feature/rollups branch from fdc5de7 to 53595bd Compare July 31, 2018 18:55

jen-huang force-pushed the feature/rollups branch from 2dc728d to 094edbd Compare August 10, 2018 18:23

cjcenizal mentioned this pull request Aug 24, 2018

[META] Rollups #20004

Closed

52 tasks

cjcenizal force-pushed the feature/rollups branch from a19825e to 5d5c431 Compare August 24, 2018 17:58

cjcenizal force-pushed the feature/rollups branch from 5d5c431 to bbb021e Compare September 4, 2018 18:35

jen-huang force-pushed the feature/rollups branch from cd48774 to 25aa4bf Compare September 7, 2018 22:20

cjcenizal force-pushed the feature/rollups branch 2 times, most recently from cb61f6a to ef9f2df Compare September 13, 2018 22:31

cjcenizal added blocked and removed blocked labels Sep 14, 2018

cjcenizal force-pushed the feature/rollups branch 2 times, most recently from 81480f2 to 0743e30 Compare September 19, 2018 00:13

maryia-lapata reviewed Sep 19, 2018

View reviewed changes

cjcenizal mentioned this pull request Sep 20, 2018

Add SearchError to courier #23382

Merged

maryia-lapata reviewed Sep 25, 2018

View reviewed changes

elastic deleted a comment from elasticmachine Sep 26, 2018

elastic deleted a comment from elasticmachine Sep 27, 2018

cjcenizal removed the WIP Work in progress label Oct 22, 2018

This comment has been minimized.

Sign in to view

jen-huang added 5 commits October 22, 2018 17:07

Convert crud less to scss

0cc042e

Remove less file

fe0789d

Remove less reference

c066660

Replace i18n function with static strings to fix reporting tests

eea3b84

Merge remote-tracking branch 'upstream/master' into feature/rollups

35dab87

spalger reviewed Oct 24, 2018

View reviewed changes

Remove delete of property on cached index pattern

0c0cfbb

cjcenizal and others added 4 commits October 24, 2018 14:18

[Rollups] Add useNormalizedEsInterval param to date histogram bucket …

a654c1d

…to allow rollups to avoid normalizing ES interval units (#24428)

Merge branch 'master' of github.com:elastic/kibana into feature/rollups

e6383d5

add rollups link to Kibana home (#24421)

ae06fc1

* add rollups link to Kibana home * copy change * copy edit * Making rollup jobs empty message match home page copy * copy edit * copy edit

Temporarily disable all endpoints other than the CRUD app.

b57c0d5

bmcconaghy approved these changes Oct 24, 2018

View reviewed changes

cjcenizal merged commit f74f633 into master Oct 25, 2018

cjcenizal mentioned this pull request Oct 25, 2018

[6.x] [Rollups] Rollup support in Kibana, phase 1 (#21117) #24554

Merged

cjcenizal removed the review label Oct 25, 2018

This was referenced Oct 29, 2018

[Rollups] Use querystring to parse crud app url #24776

Closed

[Rollups] More robust crud app API error handling #24777

Closed

cjcenizal deleted the feature/rollups branch November 1, 2018 18:47

aphelionz mentioned this pull request Nov 2, 2018

[meta] Accessibility Audit for Kibana 6.5 #24159

Closed

9 tasks

tylersmalley mentioned this pull request Dec 10, 2019

Moving saved object management to an ES plugin #49764

Closed

1 task

cjcenizal added the Feature:Rollups label Apr 23, 2020

cjcenizal mentioned this pull request Apr 23, 2020

Move Rollup out of legacy #62891

Merged

[Rollups] Rollup support in Kibana, phase 1 #21117

[Rollups] Rollup support in Kibana, phase 1 #21117

Conversation

cjcenizal commented Jul 23, 2018 • edited by jen-huang Loading

Overview

Testing plan

Rollup job management

cjcenizal commented Sep 17, 2018

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

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

maryia-lapata 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

yaronp68 commented Sep 25, 2018

yaronp68 commented Sep 25, 2018

This comment has been minimized.

elasticmachine commented Oct 23, 2018

💚 Build Succeeded

spalger 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 24, 2018

💚 Build Succeeded

bmcconaghy left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 24, 2018

💚 Build Succeeded

elasticmachine commented Oct 25, 2018

💚 Build Succeeded

cjcenizal commented Jul 23, 2018 •

edited by jen-huang

Loading

spalger Oct 24, 2018 •

edited

Loading