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

[Lens] Implement counter rate expression #82948

Merged
merged 10 commits into from
Nov 11, 2020

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Nov 9, 2020

Summary

Fixes #46627

Adds lens_counter_rate function.

The counter rate function works exactly like derivative expression if value increases or is equal to previous one – it returns the difference between current and previous value.

If value decreased compared to the previous value, the function outputs the current value. Here's the example:

input output comment
1 undefined previous value doesn't exist
5 4 increasing dif(5-1)
11 6 increasing dif(11-5)
4 4 decreasing diff, so the counter is reset to the value
4 0 dif(4-4)

Special cases

For undefined:

input output comment
1 undefined previous value doesn't exist
5 4 increasing dif(5-1)
undefined undefined undefined current value
3 undefined undefined previous value

Question: NaN case

Should value after NaN be also NaN (eg. counter between NaN and 2 - that's what derivative do) or should it be NaN?

input output comment
1 undefined previous value doesn't exist
3 2 increasing dif(3-1)
{} (converted to NaN) NaN
2 2 or NaN?? **This is the doubt**
3 1 increasing diff (3-2)

The expression takes inputColumnId, outputColumnId, outputColumnName, by (similar behavior to new expression time series functions)

Checklist

@mbondyra mbondyra added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.11.0 labels Nov 9, 2020
@mbondyra mbondyra requested a review from a team November 9, 2020 13:30
@mbondyra mbondyra requested a review from a team as a code owner November 9, 2020 13:30
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Nov 9, 2020
@flash1293
Copy link
Contributor

Should value after NaN be also NaN (eg. counter between NaN and 2 - that's that derivative do) or should it be NaN?

Good question, I think going with 2 is fine, because it's assuming broken data in the previous bucket so we are recovering as much as possible from the value in the next bucket (just like the "smaller than previous value" case)

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Relocating series_calculation_helpers outside of the specs directory makes sense to me, so I'm good on the app arch changes once we get a green build 👍

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Didn't run, but test cases look good to me. Left one small nit

const previousValue = previousValues[bucketIdentifier];
const currentValue = newRow[inputColumnId];
if (currentValue != null && previousValue != null) {
if (Number(currentValue) >= previousValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: There are four places where currentValue is cast to number, we could refactor this to a single currentValueAsNumber, but no biggie

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

I didn't run this code, but just looking at the code looks good. Left one comment about clarifying a test case.

},
{ inputColumnId: 'val', outputColumnId: 'output' }
);
expect(result.rows.map((row) => row.output)).toEqual([undefined, 7 - 5, 3, 2]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the other tests are using numbers that are always increasing, this test appears to be the only one that tests the scenario where the number goes down. Are there other scenarios to test around the edge cases of the algorithm? Or maybe you can name the test differently to indicate that this one tests 2 things?

Copy link
Contributor Author

@mbondyra mbondyra Nov 11, 2020

Choose a reason for hiding this comment

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

Good idea @wylieconlon! I'll add new test.

@mbondyra
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 911.0KB 910.4KB -567.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
expressions 181.4KB 182.3KB +862.0B
lens 50.3KB 50.6KB +238.0B
total +1.1KB

History

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

@mbondyra mbondyra merged commit a50960e into elastic:master Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[lens] Add "counter rate" for monotonically increasing numbers
6 participants