Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Add support for removeZeroSeries function #2034

Merged
merged 8 commits into from
Jun 24, 2022

Conversation

carrieedwards
Copy link
Contributor

This PR adds support for removeZeroSeries in Graphite web, which operates similarly to removeEmptySeries, but instead checks data points in the series for values equal to 0, instead of values equal to math.NaN().

@shanson7
Copy link
Collaborator

I don't see this function in graphite docs. Metrictank functions should also be in graphite.

@carrieedwards carrieedwards force-pushed the cedwards/add-removeZeroSeries-function branch from d22ee37 to 80b299e Compare June 14, 2022 16:23
@ywwg
Copy link
Contributor

ywwg commented Jun 14, 2022

This function is in go-graphite, which a customer is using so we need to support this function.

Copy link
Contributor

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

looks good, but I would like to see the empty series case tested

expr/func_removezeroseries.go Outdated Show resolved Hide resolved
expr/func_removezeroseries.go Show resolved Hide resolved
expr/func_removezeroseries_test.go Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Jun 16, 2022

CLA assistant check
All committers have signed the CLA.

@carrieedwards carrieedwards force-pushed the cedwards/add-removeZeroSeries-function branch 2 times, most recently from 255f2fb to b8976fc Compare June 24, 2022 15:07
@carrieedwards carrieedwards force-pushed the cedwards/add-removeZeroSeries-function branch from b8976fc to e95acec Compare June 24, 2022 15:08
@Jxlam-pdx
Copy link

@replay, this PR is ready to go. @carrieedwards is not authorized to merge it. Can you grant her access? thanks!

@replay
Copy link
Contributor

replay commented Jun 24, 2022

The PR should also update the table of functions here: https://github.com/grafana/metrictank/blob/master/docs/graphite.md#processing-functions

As Sean pointed out, this function doesn't exist in Graphite. How would you feel about also contributing it to Graphite after merging this PR, for parity?

@carrieedwards
Copy link
Contributor Author

I can contribute the same to Graphite as well.

@replay
Copy link
Contributor

replay commented Jun 24, 2022

I can contribute the same to Graphite as well.

that would be great, thank you

@replay replay merged commit 5a549a4 into master Jun 24, 2022
@replay replay deleted the cedwards/add-removeZeroSeries-function branch June 24, 2022 18:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants