Add Reset Transformation #2794
Merged
Add Reset Transformation #2794
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ryanhall07
force-pushed
the
rhall-reset-agg-counters
branch
2 times, most recently
from
4a26009 to
a79e480
Compare
This transformation resets an aggregated prometheus counter by emitting a zero. This transformation fixes the display issue when an M3Aggregator failover occurs. When the Add transformation is used to generate a cumulative counter for prometheus, the values in the leader and follower are different since they started counting at different times. When the failover happens, the value might decrease. When a cumulative counter decreases, PromQL assumes the counter reset. This leads to strange display issues, since the counter did not actually reset. By explicitly reseting the counter each resolution period and not accumulating a counter, PromQL can correclty graph the rate across failovers. This does have the downside of an extra 0 datapoint per resolution period. The storage cost is more than just the extra 0 since the extra 0 is stored 1 second after the actual datapoint. This degrades the timestamp encoding since the timestamps are no longer at a fixed interval. In practice we see a 3x increase in storage for these aggregated counters.
ryanhall07
force-pushed
the
rhall-reset-agg-counters
branch
from
a79e480 to
93210f8
Compare
ryanhall07
commented
Oct 24, 2020
| @@ -2,6 +2,9 @@ | |||
| mock_*.go linguist-generated | |||
| *_mock.go linguist-generated | |||
|
|
|||
| # genny files | |||
There was a problem hiding this comment.
eventually I want to fix how we generate genny files to include DO NOT EDIT, which should automatically be excluded from github. for now this quick solution works.
ryanhall07
commented
Oct 24, 2020
| @@ -1,5 +1,12 @@ | |||
| #!/bin/bash | |||
| source "${GOPATH}/src/github.com/m3db/m3/.ci/variables.sh" | |||
|
|
|||
| if [ -z "$GOPATH" ]; then | |||
There was a problem hiding this comment.
since GOPATH is deprecated, this seems like a reasonable solution
| @@ -468,10 +468,13 @@ func (e *GenericElem) processValueWithAggregationLock( | |||
| discardNaNValues = e.opts.DiscardNaNAggregatedValues() | |||
| ) | |||
| for aggTypeIdx, aggType := range e.aggTypes { | |||
| toFlush := make([]transformation.Datapoint, 0) | |||
There was a problem hiding this comment.
Can we place this slice somewhere on the element or in the locked aggregation so we can reuse it each flush? Ideally we don't need to allocate this every flush per every single series.
Alternatively, can we just create two local variables? toFlushDefault and toFlushSecondary
| // aggregated counters. | ||
| func transformReset() UnaryMultiOutputTransform { | ||
| return UnaryMultiOutputTransformFn(func(dp Datapoint) (Datapoint, []Datapoint) { | ||
| return dp, []Datapoint{{Value: 0, TimeNanos: dp.TimeNanos + int64(time.Second)}} |
There was a problem hiding this comment.
Given this always returns a fixed number of datapoints, perhaps we just special case the return type to be just one datapoint for now?
Concerned about this slice being heap allocated, can check with running go build -gcflags="-m" . and seeing what comes up for this file and whether the slice escapes to the heap or not.
There was a problem hiding this comment.
i changed it to return a single datapoint.
however, I'm still allocating a 2 element slice when processing the flush, it just made the code easier. from using go build-gcflags="-m" the slice is not escaping. however the previous code was also not escaping with
toFlush := make([]transformation.Datapoint, 0)
…all-reset-agg-counters
| this, other := reset.Evaluate(dp) | ||
| require.Equal(t, dp, this) | ||
| require.Equal(t, Datapoint{Value: 0, TimeNanos: now.Add(time.Second).UnixNano()}, other) | ||
|
|
There was a problem hiding this comment.
nit: Remove empty line?
robskillington
approved these changes
Oct 28, 2020
…all-reset-agg-counters
Codecov Report
@@ Coverage Diff @@
## master #2794 +/- ##
=========================================
- Coverage 72.1% 32.4% -39.8%
=========================================
Files 1115 731 -384
Lines 100626 76036 -24590
=========================================
- Hits 72624 24654 -47970
- Misses 23058 48909 +25851
+ Partials 4944 2473 -2471
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
soundvibe
added a commit
that referenced
this pull request
Oct 29, 2020
* master: Support dynamic namespace resolution for embedded coordinators (#2815) [dbnode] Add index regexp DFA compilation cache to avoid allocating DFAs for same expressions (#2814) [dbnode] Set default cache on retrieve to false, prepare testing with cache none (#2813) [tools] Output annotations as base64 (#2743) Add Reset Transformation (#2794) [large-tiles] Fix for a reverse index when querying downsampled namespace (#2808) [dbnode] Evict info files cache before index bootstrap in peers bootstrapper (#2802) [dbnode] Bump default filesystem persist rate limit (#2806) [coordinator] Add metrics and configurability of downsample and ingest writer worker pool (#2797) [dbnode] Fix concurrency granularity of seekerManager.UpdateOpenLease (#2790) [dbnode] Use correct import path for atomic library (#2801) Regenerate carbon automapping rules on namespaces changes (#2793) Enforce matching retention and index block size (#2783) [query] Add ClusterNamespacesWatcher and use to generate downsample automapper rules (#2782) [query][dbnode] Wire up etcd-backed Clusters implementation in coordinator (#2758) [query] Fix quantile() argument not being passed through (#2780) # Conflicts: # src/query/parser/promql/matchers.go
soundvibe
added a commit
that referenced
this pull request
Oct 29, 2020
* master: [query] Improve precision for variance and stddev of equal values (#2799) Support dynamic namespace resolution for embedded coordinators (#2815) [dbnode] Add index regexp DFA compilation cache to avoid allocating DFAs for same expressions (#2814) [dbnode] Set default cache on retrieve to false, prepare testing with cache none (#2813) [tools] Output annotations as base64 (#2743) Add Reset Transformation (#2794) [large-tiles] Fix for a reverse index when querying downsampled namespace (#2808) [dbnode] Evict info files cache before index bootstrap in peers bootstrapper (#2802) [dbnode] Bump default filesystem persist rate limit (#2806) [coordinator] Add metrics and configurability of downsample and ingest writer worker pool (#2797) [dbnode] Fix concurrency granularity of seekerManager.UpdateOpenLease (#2790) [dbnode] Use correct import path for atomic library (#2801) Regenerate carbon automapping rules on namespaces changes (#2793) Enforce matching retention and index block size (#2783) [query] Add ClusterNamespacesWatcher and use to generate downsample automapper rules (#2782) [query][dbnode] Wire up etcd-backed Clusters implementation in coordinator (#2758) [query] Fix quantile() argument not being passed through (#2780) [query] Add "median" aggregation to Graphite aggregate() function (#2774) [r2] Ensure KeepOriginal is propagated when adding/reviving rules (#2796) [dbnode] Update default db read limits (#2784)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This transformation resets an aggregated prometheus counter by emitting
a zero. This transformation fixes the display issue when an M3Aggregator
failover occurs. When the Add transformation is used to generate a cumulative
counter for prometheus, the values in the leader and follower are different
since they started counting at different times. When the failover happens, the
value might decrease. When a cumulative counter decreases, PromQL assumes the
counter reset. This leads to strange display issues, since the counter did not
actually reset. By explicitly reseting the counter each resolution period and
not accumulating a counter, PromQL can correclty graph the rate across failovers.
This does have the downside of an extra 0 datapoint per resolution period. The storage
cost is more than just the extra 0 since the extra 0 is stored 1 second after the actual
datapoint. This degrades the timestamp encoding since the timestamps are no longer at
a fixed interval. In practice we see a 3x increase in storage for these aggregated counters.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
None
Does this PR require updating code package or user-facing documentation?:
None