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.

Sign up for GitHub

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

Jump to bottom

Transformations: Cumulative and window modes for Add field from calculation #77029

Merged
merged 23 commits into from
Nov 3, 2023
Merged

Transformations: Cumulative and window modes for Add field from calculation #77029

merged 23 commits into from
Nov 3, 2023

Conversation

mdvictor
Copy link
Contributor

@mdvictor mdvictor commented Oct 24, 2023 •
edited by oscarkilhed

What is this feature?

Adds a cumulative and window functions to add field from calc
movingaverage
cumulative sum
cumulativemean

Why do we need this feature?

Provide a way to get cumulative total/mean, moving average/stddev/variance for data sources where the query model does not support it.

Who is this feature for?

All users

Which issue(s) does this PR fix?:

Fixes #74480

Special notes for your reviewer:

The transformation is getting rather big so we will need to refactor it in the future and break it up a bit. Issue for tracking here.

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@mdvictor mdvictor added add to changelog no-backport Skip backport of PR labels Oct 24, 2023
@mdvictor mdvictor added this to the 10.2.x milestone Oct 24, 2023
@grafana-delivery-bot grafana-delivery-bot bot modified the milestones: 10.2.x, 10.3.x Oct 24, 2023
mdvictor and others added 6 commits October 24, 2023 15:21
@mdvictor
refactor and create new mode
6bce8bc
@mdvictor
refactor - use reduceOptions for new mode also
bfbb2ec
@mdvictor
revert naming
d20abc5
@oscarkilhed
Add window function, rename statistical to cumulative (#77066)
c68dda1 
* Add window function, rename statistical to cumulative

* Fix merge errors

* fix more merge errors
@mdvictor @oscarkilhed
refactor + add window funcs
c7da21a 
Co-authored-by: Oscar Kilhed <oscar.kilhed@grafana.com>
@mdvictor @oscarkilhed
add ff + tests + centered moving avg
2fccec8 
Co-authored-by: Oscar Kilhed <oscar.kilhed@grafana.com>
@oscarkilhed @mdvictor
make sum and mean cumulative more efficient (#77173)
8425311 
* make sum and mean cumulative more efficient

* remove cumulative variance, add window stddev

* refactor window to not use reducer for mean. wip variance stdDev

* fix tests after optimization

---------

Co-authored-by: Victor Marin <victor.marin@grafana.com>
@mdvictor mdvictor marked this pull request as ready for review October 27, 2023 06:32
@mdvictor mdvictor requested review from grafanabot and a team as code owners October 27, 2023 06:32
@mdvictor mdvictor requested review from a team, codeincarnate and oscarkilhed and removed request for a team October 27, 2023 06:32
@mdvictor mdvictor changed the title Add cumulative sum to Add field from calc transformation Transformations: Cumulative and window modes for Add field from calculation Oct 27, 2023
oscarkilhed and others added 2 commits October 27, 2023 16:20
@oscarkilhed @mdvictor
optimize window func (#77266)
73f213a 
* make sum and mean cumulative more efficient

* remove cumulative variance, add window stddev

* refactor window to not use reducer for mean. wip variance stdDev

* fix tests after optimization

* fix test lint

* optimize window

* tests are passing

* fix nulls

* fix all nulls

---------

Co-authored-by: Victor Marin <victor.marin@grafana.com>
@mdvictor
change window size to be percentage
6d64eca
oscarkilhed
oscarkilhed reviewed Oct 27, 2023
View reviewed changes
Copy link
Contributor

@oscarkilhed oscarkilhed left a comment

Choose a reason for hiding this comment

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

Looks like the percentage thing broke the tests

@mdvictor
Copy link
Contributor Author

mdvictor commented Nov 1, 2023

I'm wondering if there is anything special we need to do with the docs since we have the new modes behind a feature flag?

@baldm0mma
Copy link
Contributor

I'm wondering if there is anything special we need to do with the docs since we have the new modes behind a feature flag?

I'm happy to set up some sort of conditional content object options based on feature flags in another PR? In the mean time, I think just mentioning in the text of the content that it's behind a FF?

@mdvictor
Copy link
Contributor Author

mdvictor commented Nov 2, 2023

I think just a text in content should be enough. Will modify

@mdvictor
Copy link
Contributor Author

mdvictor commented Nov 2, 2023

@imatwawana can I please ask for your feedback on the note I added for each of the new modes (window/cumulative) saying that it's behind a feature toggle? Would this be ok?

codeincarnate
codeincarnate approved these changes Nov 2, 2023
View reviewed changes
Copy link
Collaborator

@codeincarnate codeincarnate left a comment

Choose a reason for hiding this comment

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

Have a couple comments but honestly feels a bit like nitpicking. Tested things out and appears to be functioning great. This is awesome!

Only comment I have beyond that is that it would be nice to see some more TSDoc comments. Overall though it's pretty readable 😄

packages/grafana-data/src/transformations/transformers/calculateField.ts
@@ -46,6 +69,13 @@ const defaultReduceOptions: ReduceOptions = {
reducer: ReducerID.sum,
};

export const defaultWindowOptions: WindowOptions = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is a constant feels like it should be DEFAULT_WINDOW_OPTIONS.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a nit, but could be confusing as window is usually used to refer to interface windows.

BeverlyJaneJ
BeverlyJaneJ approved these changes Nov 3, 2023
View reviewed changes
docs/sources/panels-visualizations/query-transform-data/transform-data/index.md
Comment on lines +154 to +157
**Note:** This mode is an experimental feature. Engineering and on-call support is not available.
Documentation is either limited or not provided outside of code comments. No SLA is provided.
Enable the 'addFieldFromCalculationStatFunctions' in Grafana to use this feature.
Contact Grafana Support to enable this feature in Grafana Cloud.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**Note:** This mode is an experimental feature. Engineering and on-call support is not available.
Documentation is either limited or not provided outside of code comments. No SLA is provided.
Enable the 'addFieldFromCalculationStatFunctions' in Grafana to use this feature.
Contact Grafana Support to enable this feature in Grafana Cloud.
{{% admonition type="note" %}}
This mode is an experimental feature. Engineering and on-call support is not available. Documentation is either limited or not provided outside of code comments. No SLA is provided. Enable the 'addFieldFromCalculationStatFunctions' in Grafana to use this feature. Contact Grafana Support to enable this feature in Grafana Cloud.
{{% /admonition %}}

@mdvictor mdvictor merged commit 61d63d3 into main Nov 3, 2023
20 checks passed
@mdvictor mdvictor deleted the mdvictor/cumulative-sum branch November 3, 2023 14:40
zserge pushed a commit that referenced this pull request Nov 9, 2023
@mdvictor @oscarkilhed @zserge
Transformations: Cumulative and window modes for `Add field from calc…
9e0b4ac 
…ulation` (#77029)

* cumulative sum

* refactor and create new mode

* refactor - use reduceOptions for new mode also

* revert naming

* Add window function, rename statistical to cumulative (#77066)

* Add window function, rename statistical to cumulative

* Fix merge errors

* fix more merge errors

* refactor + add window funcs

Co-authored-by: Oscar Kilhed <oscar.kilhed@grafana.com>

* add ff + tests + centered moving avg

Co-authored-by: Oscar Kilhed <oscar.kilhed@grafana.com>

* make sum and mean cumulative more efficient (#77173)

* make sum and mean cumulative more efficient

* remove cumulative variance, add window stddev

* refactor window to not use reducer for mean. wip variance stdDev

* fix tests after optimization

---------

Co-authored-by: Victor Marin <victor.marin@grafana.com>

* optimize window func (#77266)

* make sum and mean cumulative more efficient

* remove cumulative variance, add window stddev

* refactor window to not use reducer for mean. wip variance stdDev

* fix tests after optimization

* fix test lint

* optimize window

* tests are passing

* fix nulls

* fix all nulls

---------

Co-authored-by: Victor Marin <victor.marin@grafana.com>

* change window size to be percentage

* fix tests to use percentage

* fixed/percentage window size (#77369)

* Add docs for cumulative and window functions of the add field from calculation transform. (#77352)

add docs

* splling

* change WindowType -> WindowAlignment

* update betterer

* refactor getWindowCreator

* add docs to content.ts

* add feature toggle message

---------

Co-authored-by: Oscar Kilhed <oscar.kilhed@grafana.com>
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscarkilhed oscarkilhed oscarkilhed left review comments

@codeincarnate codeincarnate codeincarnate approved these changes

@BeverlyJaneJ BeverlyJaneJ BeverlyJaneJ approved these changes

@grafanabot grafanabot Awaiting requested review from grafanabot grafanabot is a code owner

@imatwawana imatwawana Awaiting requested review from imatwawana imatwawana is a code owner

@leeoniya leeoniya Awaiting requested review from leeoniya

@baldm0mma baldm0mma Awaiting requested review from baldm0mma baldm0mma is a code owner

Assignees
No one assigned
Labels
add to changelog add to what's new area/backend area/frontend no-backport Skip backport of PR type/docs
Projects
None yet
Milestone
10.2.3
7 participants
@mdvictor @baldm0mma @leeoniya @codeincarnate @oscarkilhed @BeverlyJaneJ @aangelisc