Fix issue with long values in TSVB static metric #40256

timroes · 2019-07-03T12:37:56Z

Summary

This makes sure that the user can also enter large static values, by using the long suffix in Painless/Java for all integer values. Since the user should still be able to use decimal numbers, we must check beforehand if the value is an integer and if not use the value as it is (double in painless).

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-07-03T12:37:58Z

Pinging @elastic/kibana-app

elasticmachine · 2019-07-03T13:37:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alexwizp · 2019-07-05T07:51:21Z

src/legacy/core_plugins/metrics/server/lib/vis_data/helpers/bucket_transform.js

@@ -78,11 +78,12 @@ export const bucketTransform = {
  },
  static: bucket => {
    checkMetric(bucket, ['value']);
+    const isDecimalValue = !Number.isInteger(Number(bucket.value));


Maybe we can move it into helper 'method' to reuse it in future.
Also looks like it returns 'true' if bucket.value is string.
!Number.isInteger(Number('stringValue')); /// => true

I don't think this line would justify a utility method, especially as you mentioned it's not very generic, since it works because we know that this will be a valid number in this place, generated by TSVB.

what about to use RegExp here instead?

Why would you prefer using regex, if we can use a native Number method to check the same?

I know that we "sure" that the number in this place is a valid number, but I never trust code, so I'd also like to have a small check for NaN values here. It doesn't hurt the code having one more check

markov00

Code LGTM, I'd prefer one more check on that bucket.value instead of a blind trust on the number correctness

markov00 · 2019-07-05T08:51:25Z

src/legacy/core_plugins/metrics/server/lib/vis_data/helpers/bucket_transform.js

@@ -78,11 +78,12 @@ export const bucketTransform = {
  },
  static: bucket => {
    checkMetric(bucket, ['value']);
+    const isDecimalValue = !Number.isInteger(Number(bucket.value));


I know that we "sure" that the number in this place is a valid number, but I never trust code, so I'd also like to have a small check for NaN values here. It doesn't hurt the code having one more check

alexwizp · 2019-07-05T09:41:52Z

src/legacy/core_plugins/metrics/server/lib/vis_data/helpers/bucket_transform.js

@@ -78,11 +78,13 @@ export const bucketTransform = {
  },
  static: bucket => {
    checkMetric(bucket, ['value']);
+    // Anything containing a decimal point or an exponent is considered decimal value
+    const isDecimalValue = bucket.value.match(/[.e]/i);


👍
But probably it make sense to wrap it Boolean(bucket.value.match(/[.e]/i)) . In case of false isDecimalValue === undefined

Also this solution works only if bucket.value has string value. I still think that we can create a helper method for that because the following code can confuse
Boolean(Number(bucket.value).toString().match(/[.e]/i))

As long as we're inside JS I think checking for null as a falsey value is fine, but I can wrap it. I cannot convert it to a number, because of the issue you pointed out that it will actually create the 64 bit rounding issues for the check, but then when we later write the script the string value won't have those, so Number('1231123112631231233.123').toString() == "1231123112631231200" meaning we will append a L and then Elasticsearch will fail because it's not a valid long.

elasticmachine · 2019-07-05T09:42:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2019-07-05T09:42:56Z

Jenkins, test this - timeout issue

elasticmachine · 2019-07-05T11:10:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

markov00

LGTM!

…ic-long

elasticmachine · 2019-07-05T14:58:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Fix issue with long values in TSVB static metric * Handle numeric values with decimal zeros * Work properly with exponential values * Wrap into boolean

Tim Roes added 2 commits July 3, 2019 14:27

Fix issue with long values in TSVB static metric

cf38f1c

Handle numeric values with decimal zeros

4489a7a

timroes added Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.3.0 labels Jul 3, 2019

timroes requested review from emmacunningham, markov00 and a team July 3, 2019 12:37

spalger added the v7.4.0 label Jul 3, 2019

alexwizp reviewed Jul 5, 2019

View reviewed changes

alexwizp approved these changes Jul 5, 2019

View reviewed changes

markov00 approved these changes Jul 5, 2019

View reviewed changes

Work properly with exponential values

a8b3cb4

timroes requested review from markov00 and alexwizp July 5, 2019 09:10

alexwizp reviewed Jul 5, 2019

View reviewed changes

Wrap into boolean

ac2028b

markov00 approved these changes Jul 5, 2019

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into fix-tsvb-stat…

9db6b55

…ic-long

timroes merged commit b7de2ac into elastic:master Jul 5, 2019

timroes deleted the fix-tsvb-static-long branch July 5, 2019 14:59

timroes mentioned this pull request Jul 5, 2019

[7.x] Fix issue with long values in TSVB static metric (#40256) #40430

Merged

timroes mentioned this pull request Jul 5, 2019

[7.3] Fix issue with long values in TSVB static metric (#40256) #40431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with long values in TSVB static metric #40256

Fix issue with long values in TSVB static metric #40256

timroes commented Jul 3, 2019

elasticmachine commented Jul 3, 2019

elasticmachine commented Jul 3, 2019

alexwizp Jul 5, 2019

timroes Jul 5, 2019

alexwizp Jul 5, 2019

timroes Jul 5, 2019

markov00 Jul 5, 2019

markov00 left a comment

markov00 Jul 5, 2019

alexwizp Jul 5, 2019 •

edited

Loading

timroes Jul 5, 2019

elasticmachine commented Jul 5, 2019

timroes commented Jul 5, 2019

elasticmachine commented Jul 5, 2019

markov00 left a comment

elasticmachine commented Jul 5, 2019

Fix issue with long values in TSVB static metric #40256

Fix issue with long values in TSVB static metric #40256

Conversation

timroes commented Jul 3, 2019

Summary

Checklist

For maintainers

elasticmachine commented Jul 3, 2019

elasticmachine commented Jul 3, 2019

💚 Build Succeeded

alexwizp Jul 5, 2019

Choose a reason for hiding this comment

timroes Jul 5, 2019

Choose a reason for hiding this comment

alexwizp Jul 5, 2019

Choose a reason for hiding this comment

timroes Jul 5, 2019

Choose a reason for hiding this comment

markov00 Jul 5, 2019

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

markov00 Jul 5, 2019

Choose a reason for hiding this comment

alexwizp Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

timroes Jul 5, 2019

Choose a reason for hiding this comment

elasticmachine commented Jul 5, 2019

💔 Build Failed

timroes commented Jul 5, 2019

elasticmachine commented Jul 5, 2019

💚 Build Succeeded

markov00 left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 5, 2019

💚 Build Succeeded

alexwizp Jul 5, 2019 •

edited

Loading