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

[timelion] allow sum, subtract, multiply, divide functions to accept seriesList with multiple series #14891

Merged
merged 10 commits into from Dec 5, 2017

Conversation

@nreese
Copy link
Contributor

commented Nov 10, 2017

fixes #13781

Allow something like:

.es(
	index='*', timefield='field', metric='sum:int1', split='user:10'
).subtract(.es(
	index='*', timefield='field', metric='sum:int2', split='user:10'
))

Another example .es(index=logstash-*,split=machine.os.raw:5).add(.es(index=logstash-*,split=machine.os.raw:5))

SeriesList are applied label-wise to each other resulting in one series per split.

@thomasneirynck
Copy link
Contributor

left a comment

This is a great improvement, makes a lot of sense intuitively, 👍


The behavior, when the labels in the split-series are not equal, seems a little undefined for a couple of use-cases.

Consider two scenarios (all use logstash example data):

  1. labels are identical but from different series

consider these two split-series , which are identical:

image

We cannot substract them:

image

Fails with:

 Error: series could not be found for label q:* > geo.dest:CN > sum(bytes)
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:34:17
    at next (native)
    at step (/home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:13:191)
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:13:437
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:13:99
    at seriesList.list.forEach (/home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:32:7)
    at Array.forEach (native)
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:32:23
    at next (native)
    at step (/home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:13:191)
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:13:361
  1. labels are not identical

these two series are different:
image

but subtracting them (which you probably don't want) doesn't yield any useful info

image

Fails with:

server    log   [21:29:26.869] [warning][process] Error: series could not be found for label q:* > machine.os.raw:win 7 > sum(bytes)
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:34:17
    at next (native)
    at step (/home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:13:191)
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:13:437
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:13:99
    at seriesList.list.forEach (/home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:32:7)
    at Array.forEach (native)
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:32:23
    at next (native)
    at step (/home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:13:191)
    at /home/thomas/repos/kibana/src/core_plugins/timelion/server/lib/reduce.js:13:361

Maybe we should only apply this:

  • when the length is equal
  • and the labels are equal (not the full series-name, but only the label)
  • and the label order is identical

If those cases are not met, we should surface the error. What do you think?

@nreese

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

Why the third case (label order is identical)?

@nreese nreese force-pushed the nreese:multiple_series_in_series_list branch from 1fdd20d to c21fed0 Nov 17, 2017

@thomasneirynck thomasneirynck self-requested a review Nov 17, 2017

@nreese

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

@thomasneirynck Looks like comparing the labels is a bad idea. The labels contain the metric name and query string so they don't match if you are looking at different metrics or different queries. The split label will have to be added to the seriesList so reduce can compare that label without all of the other stuff. I updated how timelion turns aggregation results into seriesList and added a split field. Then reduce does its label comparison on that field. Now queries like the one below work.

.es(index=logstash-*,split=machine.os.raw:5,q="bytes: [5000 TO 20000]")
.add(.es(index=logstash-*,split=machine.os.raw:5))

@nreese nreese added v6.2.0 and removed v6.1.0 labels Nov 21, 2017

@nreese nreese force-pushed the nreese:multiple_series_in_series_list branch from ff7f068 to 5bef751 Nov 27, 2017

@nreese

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

@thomasneirynck @ppisljar This PR is ready for another look.

@ppisljar
Copy link
Member

left a comment

LGTM, i think it gives nice capabilities, and when you type something that wouldn't work you get the notification warning.

by the way, thomas in the examples above: why would you be doing the split on two different fields, whats the use case?

@thomasneirynck

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2017

@ppisljar I think it might be useful to run a calculation on two different series when the concept in the two fields are the same. say e.g. to a delta comparison between sum of two fields, split over a couple of terms. Those terms may be the same concept, e.g. zipcode, country-code, username, ...

@ppisljar

This comment has been minimized.

Copy link
Member

commented Dec 5, 2017

@thomasneirynck i agree, but i guess you should always split on a same field ... so first serie would be sum of bytes (split on top 5 countries) and second series might be the count (again on the same split). Dont see the usecase where you would want to first split on geo.src and in then on geo.dest ... it won't give you same top5 countries, so it won't make any sense to divide bytes from US by count from china ?

@nreese nreese force-pushed the nreese:multiple_series_in_series_list branch from 865641d to b1888df Dec 5, 2017

@thomasneirynck

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2017

@ppisljar makes sense.that example seems not a real-world use-case after all. i thought ordering could also be determined by the terms themselves.

@nreese nreese merged commit d1550c1 into elastic:master Dec 5, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

nreese added a commit to nreese/kibana that referenced this pull request Dec 5, 2017

[timelion] allow sum, subtract, multiply, divide functions to accept …
…seriesList with multiple series (elastic#14891)

* allow seriesList with multiple series

* remove junk file

* fix promise bug in precision.js

* remove another junk file

* throw error outside of async function so it is properly handled

* compare split label and not entire series label which includes metric and query strings

* reduce seriesList by label field when both seriesList do not contain splitKey field

* use clearer variable names

* move pairwiseReduce to its own function

* fix es.js test

nreese added a commit that referenced this pull request Dec 5, 2017

[timelion] allow sum, subtract, multiply, divide functions to accept …
…seriesList with multiple series (#14891) (#15413)

* allow seriesList with multiple series

* remove junk file

* fix promise bug in precision.js

* remove another junk file

* throw error outside of async function so it is properly handled

* compare split label and not entire series label which includes metric and query strings

* reduce seriesList by label field when both seriesList do not contain splitKey field

* use clearer variable names

* move pairwiseReduce to its own function

* fix es.js test
@rbosneag

This comment has been minimized.

Copy link

commented Jan 5, 2018

What is the first release this PR made it into? I looked everywhere and couldn't find anything like it.
Sorry for being off topic...

@nreese

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2018

It will be in the next released version - 6.2

@rbosneag

This comment has been minimized.

Copy link

commented Jan 5, 2018

Thanks!

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.