Add dateHistogramInterval utility #39091

timroes · 2019-06-17T16:55:19Z

Summary

Adds the dateHistogramInterval utility, which can be used to calculate the correct interval key (calendar_interval or fixed_interval) for a given interval string (like 10s, 1h, etc.).

The return will be an object that can simply spreaded into a date_histogram body. So you should replace your previous interval key by a call to that function:

// You can also use an relative import to the path `src/legacy/core_plugins/data/common` instead
+ import { dateHistogramInterval } from 'plugins/data/common';

const aggregation = {
  date_histogram: {
    field: '@timestamp',
-   interval: '10s',
+   ...dateHistogramInterval('10s'),
  },
};

This function will throw an error if the interval itself is not a valid Elasticsearch interval (like 2w, 5.0h, ...). Please make sure if you use it for user input, that the user input is properly validated beforehand and you show the user a meaningful error message. You can use the isValidEsInterval function as follows:

import { isValidEsInterval } from 'plugins/data/common';

const isValid: boolean = isValidEsInterval('10s');

Release Summary: Migrate date_histograms to the new calendar_interval/fixed_interval. Solves some deprecation warnings in Kibana.

For reviewers

This PR adds a bit of tech-debt, since we import directly from the plugins/data/common folder (which is not one of the root folders we should import later on). Unfortunately there are still a couple of unsolved issues around testing and NP plugins, why we decided to for now introduce that tech-debt, especially since it's really easy to change those imports later on.

This PR also makes use of this method in courier via AggConfig and also makes sure the editor properly validates that value beforehand.

There are a lot of test fixes, now that the AggConfig actually needs a valid interval since it would otherwise throw an exception.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-06-17T17:38:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2019-06-17T17:38:27Z

Jenkins, test this - short CI outage

elasticmachine · 2019-06-17T18:38:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-17T19:20:26Z

Pinging @elastic/kibana-app

elasticmachine · 2019-06-17T19:20:28Z

Pinging @elastic/kibana-app-arch

markov00 · 2019-06-20T10:53:40Z

cc @emmacunningham FYI (replacement of discover chart)

flash1293

Code LGTM. Is there a specific reason to re-export this under static and common? Will this be a convention in NP?

jbudz · 2019-06-24T13:05:40Z

src/dev/jest/config.js

@@ -53,7 +53,7 @@ export default {
    '!src/legacy/core_plugins/**/__snapshots__/**/*',
  ],
  moduleNameMapper: {
-    '^plugins/([^\/.]*)/(.*)': '<rootDir>/src/legacy/core_plugins/$1/public/$2',
+    '^plugins/([^\/.]*)(.*)': '<rootDir>/src/legacy/core_plugins/$1/public$2',


context is to allow nested folders?

rather the opposite, to allow importing from the plugin directly, like plugin/data, but to be honest that's not needed anymore so I can actually revert that change here.

Well but since we might hit that problem another time I might just leave it, since you anyway already approved it :)

jbudz

jest config LGTM

elasticmachine · 2019-06-24T14:11:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-24T14:51:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-24T16:29:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers

Just one last question :)

lukeelmers · 2019-06-24T21:48:36Z

src/legacy/ui/public/agg_types/utils.ts

@@ -17,7 +17,7 @@
 * under the License.
 */

-import { parseInterval } from '../utils/parse_interval';
+import { isValidEsInterval } from '../../../core_plugins/data/common';


Are you importing from data/common here instead of data/public to avoid the mocking issues that were surfacing due to all of the stuff that's imported into data/public/index?

Because otherwise I'd suggest importing from data/public since common will be an implementation detail of the plugin in the new platform, and may change.

Yes that was exactly the fix (that I didn't remember in our last meeting). So basically because of the mocking issues I import directly from common. I also added to the PR description that this is a tech-debt we need to remove later on, once we have a proper solution for the mocking issue with one large entry file (or then potentially another solution).

cc @skaapgif @joshdover This was actually the workaround I used for now, which I forgot about in our last meeting, how I fixed it in this PR :) So simply deep importing from not the top level entry file.

* Add dateHistogramInterval utility * Fix editor bug * Fix tests * FIx imports to temporary solution * Remove wrongly merged translations * Remove old static.ts file

elasticmachine · 2019-06-26T10:45:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes added 4 commits June 17, 2019 16:25

Add dateHistogramInterval utility

df6f942

Fix editor bug

bfefb51

Fix tests

19048a2

FIx imports to temporary solution

3bdc700

timroes mentioned this pull request Jun 17, 2019

Add dateHistogramInterval utility #37108

Closed

7 tasks

timroes requested review from mattkime, lizozom and lukasolson June 17, 2019 19:14

timroes added Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Team:AppArch Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jun 17, 2019

timroes added v7.3.0 v8.0.0 labels Jun 17, 2019

timroes requested review from flash1293 and removed request for mattkime and lizozom June 17, 2019 19:20

timroes marked this pull request as ready for review June 17, 2019 19:24

timroes requested a review from a team as a code owner June 17, 2019 19:24

timroes requested a review from Bargs June 17, 2019 19:31

timroes added the release_note:enhancement label Jun 17, 2019

timroes requested a review from lukeelmers June 18, 2019 11:31

flash1293 approved these changes Jun 21, 2019

View reviewed changes

jbudz reviewed Jun 24, 2019

View reviewed changes

jbudz approved these changes Jun 24, 2019

View reviewed changes

timroes added 2 commits June 24, 2019 15:10

Merge branch 'master' into date-histogram-interval

1af4102

Remove wrongly merged translations

08d7d27

Remove old static.ts file

7059852

lukeelmers approved these changes Jun 24, 2019

View reviewed changes

timroes merged commit 850e7fb into elastic:master Jun 25, 2019

timroes deleted the date-histogram-interval branch June 25, 2019 07:32

timroes mentioned this pull request Jun 25, 2019

[7.x] Add dateHistogramInterval utility (#39091) #39563

Merged

$@polyfractal$ polyfractal mentioned this pull request Jun 25, 2019

Interval field on DateHistograms will be deprecated #27410

Closed

4 tasks

alexwizp mentioned this pull request Jun 26, 2019

[TSVB] Apply dateHistogramInterval utility #39665

Merged

7 tasks

This was referenced Jul 8, 2019

App Architecture roadmap for New Platform migration #31968

Closed

[New Platform Migration Phase I]: update dateHisogramInterval & parseEsInterval imports #40553

Closed

alexwizp mentioned this pull request Aug 8, 2019

[New Platform Migration Phase I]: update dateHisogramInterval & parseEsInterval imports #42917

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dateHistogramInterval utility #39091

Add dateHistogramInterval utility #39091

timroes commented Jun 17, 2019 •

edited

Loading

elasticmachine commented Jun 17, 2019

timroes commented Jun 17, 2019

elasticmachine commented Jun 17, 2019

elasticmachine commented Jun 17, 2019

elasticmachine commented Jun 17, 2019

markov00 commented Jun 20, 2019

flash1293 left a comment

jbudz Jun 24, 2019

timroes Jun 24, 2019

timroes Jun 24, 2019

jbudz left a comment

elasticmachine commented Jun 24, 2019

elasticmachine commented Jun 24, 2019

elasticmachine commented Jun 24, 2019

lukeelmers left a comment

lukeelmers Jun 24, 2019

timroes Jun 25, 2019

elasticmachine commented Jun 26, 2019

Add dateHistogramInterval utility #39091

Add dateHistogramInterval utility #39091

Conversation

timroes commented Jun 17, 2019 • edited Loading

Summary

For reviewers

Checklist

For maintainers

elasticmachine commented Jun 17, 2019

💔 Build Failed

timroes commented Jun 17, 2019

elasticmachine commented Jun 17, 2019

💚 Build Succeeded

elasticmachine commented Jun 17, 2019

elasticmachine commented Jun 17, 2019

markov00 commented Jun 20, 2019

flash1293 left a comment

Choose a reason for hiding this comment

jbudz Jun 24, 2019

Choose a reason for hiding this comment

timroes Jun 24, 2019

Choose a reason for hiding this comment

timroes Jun 24, 2019

Choose a reason for hiding this comment

jbudz left a comment

Choose a reason for hiding this comment

elasticmachine commented Jun 24, 2019

💚 Build Succeeded

elasticmachine commented Jun 24, 2019

💚 Build Succeeded

elasticmachine commented Jun 24, 2019

💚 Build Succeeded

lukeelmers left a comment

Choose a reason for hiding this comment

lukeelmers Jun 24, 2019

Choose a reason for hiding this comment

timroes Jun 25, 2019

Choose a reason for hiding this comment

elasticmachine commented Jun 26, 2019

💚 Build Succeeded

timroes commented Jun 17, 2019 •

edited

Loading