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] Math params._interval is incorrect when using entire timerange mode #100775

Merged
merged 7 commits into from
Jun 3, 2021

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented May 27, 2021

Closes: #100615
Closes: #101022

Summary

params._interval is used to normalize the display of values, for example to get an average rate per second. When using the entire timerange mode, it should represent the total number of milliseconds in the time range.

Steps to reproduce:

  1. Set the following value for the Timerange control
    image

  2. Open TSVB and add a Math function. Type params._interval into the math input.
    image

  3. Switch to the Metric visualization type.

  4. Open Panel Options, then choose "Last value" timerange mode instead. Notice that the interval is the same.

Expected behavior: The params._interval is set to the number of milliseconds in the query, the to - from time.

Screens

  • Entire time range mode
    image

  • Last value mode
    image

@@ -86,8 +86,7 @@ describe('dateHistogram(req, panel, series)', () => {
},
},
meta: {
bucketSize: 10,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bucketSize was removed to avoid of any calculation in future, cause in fact we need a interval

@@ -8,9 +8,9 @@

import { mathAgg } from '../series/math';

export function math(bucket, panel, series) {
export function math(bucket, panel, series, meta, extractFields) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a separate PR (#100765) was created for that change which should be backported into 7.13.2

@alexwizp alexwizp requested a review from wylieconlon May 27, 2021 13:40
@alexwizp alexwizp self-assigned this May 27, 2021
@alexwizp alexwizp added v7.14.0 v8.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix labels May 27, 2021
@alexwizp alexwizp added the auto-backport Deprecated - use backport:version if exact versions are needed label May 27, 2021
Copy link
Contributor

@Kuznietsov Kuznietsov left a comment

Choose a reason for hiding this comment

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

LGTM. Tested locally, everything works as expected.

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp alexwizp marked this pull request as ready for review May 31, 2021 13:11
@alexwizp alexwizp requested a review from a team May 31, 2021 13:11
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@stratoula
Copy link
Contributor

@alexwizp I think the same problem with Math also happens with Bucket script. Do you think we could solve this in this PR?

const body = {
derivative: {
buckets_path: getBucketsPath(bucket.field, metrics),
gap_policy: 'skip', // seems sane
unit: bucketSize,
unit: intervalString,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to the documentation there should be a string value here, not the size of the bucket

https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-pipeline-derivative-aggregation.html#_units

Copy link
Contributor

@VladLasitsa VladLasitsa left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally in chrome

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 261 149 -112
lens 67 45 -22
licensing 18 15 -3
lists 239 236 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
securitySolution 390 346 -44
stackAlerts 101 95 -6
total -342

History

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

cc @alexwizp

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.

My dear Alexey, math and bucket script seem to work ok now. I did some tests and I think we are fine

@alexwizp alexwizp merged commit 83b47c1 into elastic:master Jun 3, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 3, 2021
… mode (elastic#100775)

* [TSVB] Math params._interval is incorrect when using entire timerange mode

Closes: elastic#100615

* fix jest

* rename get -> overwrite

* apply fix for "bucket script"

* Update date_histogram.js

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 3, 2021
… mode (#100775) (#101252)

* [TSVB] Math params._interval is incorrect when using entire timerange mode

Closes: #100615

* fix jest

* rename get -> overwrite

* apply fix for "bucket script"

* Update date_histogram.js

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

Co-authored-by: Alexey Antonov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0
Projects
None yet
6 participants