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

Add support for date math in Timelion's .movingaverage() #11555

Merged
merged 3 commits into from May 12, 2017

Conversation

Projects
None yet
5 participants
@rashidkpc
Copy link
Member

commented May 1, 2017

Purpose 1
This adds support for date math, eg 1m, in timelion's .movingaverage() function.

For many people, date math will actually make more sense than the normal point-wise method as it make the time domain the only thing you have to care about. It's exceptionally useful for those users that get data at a fixed interval and that want to smooth out spikey data in a standard unit of time, such as: https://discuss.elastic.co/t/bytes-per-second-is-it-possible/83497/11

datemath_mvavg

Purpose 2
This also fixes an off-by-1 bug in movingaverage() that caused a phase shift in "left" alignment and a dropped point in "right" and "center".

screen shot 2017-05-01 at 3 44 38 pm

@rashidkpc rashidkpc self-assigned this May 1, 2017

@rashidkpc rashidkpc requested a review from ppisljar May 1, 2017

@rashidkpc rashidkpc force-pushed the rashidkpc:timelion/moving_datemath branch from f10e5c3 to 9ce97a1 May 1, 2017

@ppisljar
Copy link
Member

left a comment

LGTM

@thomasneirynck thomasneirynck self-requested a review May 3, 2017

@rashidkpc

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

@thomasneirynck Did you still want to review this? I'd like to get it merge for 5.5

@thomasneirynck
Copy link
Contributor

left a comment

Love the added convenience of being able to specify intervals by time.

Only some minor style feedback, for your consideration.


// Round, floor, ceil? We're going with round because it splits the difference.
return Math.round(windowMilliseconds / intervalMilliseconds) || 1;
}());

This comment has been minimized.

Copy link
@thomasneirynck

thomasneirynck May 11, 2017

Contributor

Consider using a simple if-statement to reassign the _window variable. A self-executing function like this is quite advanced usage and has little advantage now that we have block-scoping in JS.

e.g.

if (typeof_window !== 'number'){
    const windowMilliseconds = ....
    ...etc...
}
types: ['number', 'string'],
help: 'Number of points, or a date math expression (eg 1d, 1M) to average over. ' +
'If you specify a date math expression, I\'ll get as close as I can given your current interval selection' +
'If they don\'t divide nicely things might get weird.'

This comment has been minimized.

Copy link
@thomasneirynck

thomasneirynck May 11, 2017

Contributor

Very conversational language, the first person perspective :). I don't think we use this consistently in the help-docs of Timelion (quandl, static are the only ones it seems like), so consider rephrasing.

@rashidkpc

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

@thomasneirynck comments addressed. I'm going to go ahead and merge this once the tests pass

@rashidkpc

This comment has been minimized.

Copy link
Member Author

commented May 12, 2017

jenkins, test this

@rashidkpc rashidkpc merged commit 01e5402 into elastic:master May 12, 2017

2 checks passed

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

snide added a commit to snide/kibana that referenced this pull request May 30, 2017

Add support for date math in Timelion's .movingaverage() (elastic#11555)
* Add support for date math to movingaverage

* Fix excess phase shift and dropped point in .movingaverage()

* Change help language, use if() instead of self executing function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.