chore: optimise rate calculation #810

yhl25 · 2023-06-28T09:36:30Z

Closes #788

Optimized rate calculation as a slope ~ delta(count)/delta(t), instead of summing the count deltas between each of the timestamp interval pairs
Optimized the search for startIndex of TimestampedCounts from linear to binary
Rate calculation remains linear in case of pod restart, which is a rare case

Signed-off-by: veds-g <guptavedant2312@gmail.com>

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

Signed-off-by: veds-g <guptavedant2312@gmail.com>

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

Signed-off-by: veds-g <guptavedant2312@gmail.com>

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

… metrics-multipart

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

Signed-off-by: veds-g <guptavedant2312@gmail.com>

…ipart

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

Signed-off-by: veds-g <guptavedant2312@gmail.com>

pkg/daemon/server/service/rater/helper.go

Signed-off-by: veds-g <guptavedant2312@gmail.com>

whynowy · 2023-07-06T19:09:13Z

Can anybody explain to me why we introduced the concept of WidowClosed?

vigith · 2023-07-06T19:14:55Z

Can anybody explain to me why we introduced the concept of WidowClosed?

#795. Ideally, this should be removed by #788

whynowy · 2023-07-06T20:02:41Z

Can anybody explain to me why we introduced the concept of WidowClosed?

#795. Ideally, this should be removed by #788

I was expecting the delta calculation happens during GetRate() which is memory operation only, would not cause any performance issue, there's no need to introduce the complexity to calculate it during metrics fetching.

vigith · 2023-07-06T20:06:37Z

I was expecting the delta calculation happens during GetRate() which is memory operation only, would not cause any performance issue, there's no need to introduce the complexity to calculate it during metrics fetching.

Won't GetRate be called frequently for auto-scaling?

whynowy · 2023-07-06T20:29:01Z

I was expecting the delta calculation happens during GetRate() which is memory operation only, would not cause any performance issue, there's no need to introduce the complexity to calculate it during metrics fetching.

Won't GetRate be called frequently for auto-scaling?

3 times in 30 seconds.

vigith · 2023-07-06T16:06:17Z

pkg/daemon/server/service/rater/timestamped_counts.go

-
-	// finalize the window by setting isWindowClosed to true and delta to the calculated value
+// CloseWindow closes the window
+func (tc *TimestampedCounts) CloseWindow() {


do we still need this?

we can remove this since the window is not closed only for the latest timestamp (last element of the queue) which we skip for rate calculation for the same reason.

vigith · 2023-07-06T16:08:52Z

pkg/daemon/server/service/rater/helper.go

+		items[0].CloseWindow()
 	default:
 		// if the queue has more than one element, we close the window for the most recent element
-		items[n-1].CloseWindow(items[n-2])
+		items[n-1].CloseWindow()


we are calling CloseWindow for all the cases? is it required?

veds-g · 2023-07-07T04:33:39Z

Can anybody explain to me why we introduced the concept of WidowClosed?

#795. Ideally, this should be removed by #788

I was expecting the delta calculation happens during GetRate() which is memory operation only, would not cause any performance issue, there's no need to introduce the complexity to calculate it during metrics fetching.

Delta calculation only happens once GetRates() is called, prior to that just the counts are fetched and updated in the queue.

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

Signed-off-by: veds-g <guptavedant2312@gmail.com>

KeranYang · 2023-07-12T13:44:09Z

pkg/daemon/server/service/rater/helper.go

-	// TODO: revisit this logic, we can just use the slope (counts[endIndex] - counts[startIndex] / timeDiff) to calculate the rate.
+	rate := getDeltaBetweenTimestampedCounts(counts[startIndex], counts[endIndex], partitionName) / float64(timeDiff)
+
+	// positive slope, meaning there was no restart in the last lookback seconds


positive slope, meaning there was no restart in the last lookback seconds

@yhl25 @veds-g , does this statement hold true when a pod restarted and then processed more messages than before restart?

Consider a single pod single partition scenario and let's assume the pod process 5 messages per second.
t1 -> 5
t2-> 10
t3 -> 0 (restart)
t4 -> 5
t5 -> 10
t6 -> 15

In this case, getDeltaBetweenTimestampedCounts(t1, t6) returns a positive delta but that doesn't mean no restart, right? It will also return us an incorrect rate of (15-5)/5 = 2 messages/second ?

this is indeed a problem; for now, i overlooked it because this can happen only if the pods are flapping, which will cause the prev value before restart to be closer to the current value.

please open an issue to track this, this has to be fixed.

My opinion is, still use the approach of calculating each delta, again, it's just memory operation, won't have performance issue.

@whynowy +1. I will use a separate PR to address. #847

veds-g and others added 15 commits June 22, 2023 18:23

multipartiton metrics changes

b0265ac

Signed-off-by: veds-g <guptavedant2312@gmail.com>

more changes

6f7d536

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

cleanup, fixing tests

8b3b6da

Signed-off-by: veds-g <guptavedant2312@gmail.com>

fix pending metrics

ee36f7c

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

metrics supports across pods for non-reduce, tests fix

b44f2fa

Signed-off-by: veds-g <guptavedant2312@gmail.com>

calculate pending per partition

9b11ed8

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

Merge branch 'metrics-multipart' of github.com:numaproj/numaflow into…

fc542f0

… metrics-multipart

lint

48cd073

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

fix npe

f648d3f

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

comments

7fadd94

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

fixed tests, comments

e6bc96b

Signed-off-by: veds-g <guptavedant2312@gmail.com>

inconsistent rate fix after vertex pods deletion

1741b84

Signed-off-by: veds-g <guptavedant2312@gmail.com>

adding tests

b2bee19

Signed-off-by: veds-g <guptavedant2312@gmail.com>

Merge branch 'main' into metrics-multipart

28ddb14

Merge branch 'main' of github.com:numaproj/numaflow into metrics-mult…

3e065af

…ipart

yhl25 assigned veds-g Jun 28, 2023

yhl25 added 2 commits June 28, 2023 22:46

Merge branch 'main' of github.com:numaproj/numaflow into optimise-rate

279e39e

optimise rater

698a182

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

yhl25 force-pushed the optimise-rate branch from 2f7c6e9 to 698a182 Compare June 28, 2023 17:17

optimize startIdx search, fix tests

f57130f

Signed-off-by: veds-g <guptavedant2312@gmail.com>

yhl25 commented Jul 6, 2023

View reviewed changes

pkg/daemon/server/service/rater/helper.go Outdated Show resolved Hide resolved

yhl25 commented Jul 6, 2023

View reviewed changes

pkg/daemon/server/service/rater/helper.go Outdated Show resolved Hide resolved

addressing review comments

d2ec398

Signed-off-by: veds-g <guptavedant2312@gmail.com>

yhl25 marked this pull request as ready for review July 6, 2023 06:08

yhl25 requested review from whynowy and vigith as code owners July 6, 2023 06:08

vigith reviewed Jul 7, 2023

View reviewed changes

yhl25 added 2 commits July 7, 2023 10:10

remove window concept

1298ea9

Signed-off-by: Yashash H L <yashashhl25@gmail.com>

Merge branch 'main' into optimise-rate

2f22079

yhl25 requested a review from vigith July 7, 2023 05:28

nits

ed94947

Signed-off-by: veds-g <guptavedant2312@gmail.com>

vigith approved these changes Jul 7, 2023

View reviewed changes

veds-g merged commit 5639e5c into main Jul 7, 2023
14 checks passed

veds-g deleted the optimise-rate branch July 7, 2023 15:04

KeranYang reviewed Jul 12, 2023

View reviewed changes

KeranYang mentioned this pull request Jul 12, 2023

Fix potential incorrect processing rate calculation #846

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: optimise rate calculation #810

chore: optimise rate calculation #810

yhl25 commented Jun 28, 2023 •

edited by veds-g

Loading

whynowy commented Jul 6, 2023

vigith commented Jul 6, 2023

whynowy commented Jul 6, 2023

vigith commented Jul 6, 2023

whynowy commented Jul 6, 2023

vigith Jul 6, 2023

veds-g Jul 7, 2023

vigith Jul 6, 2023

yhl25 Jul 7, 2023

veds-g commented Jul 7, 2023

KeranYang Jul 12, 2023 •

edited

Loading

vigith Jul 12, 2023

KeranYang Jul 12, 2023

whynowy Jul 12, 2023

KeranYang Jul 12, 2023 •

edited

Loading

chore: optimise rate calculation #810

chore: optimise rate calculation #810

Conversation

yhl25 commented Jun 28, 2023 • edited by veds-g Loading

whynowy commented Jul 6, 2023

vigith commented Jul 6, 2023

whynowy commented Jul 6, 2023

vigith commented Jul 6, 2023

whynowy commented Jul 6, 2023

vigith Jul 6, 2023

Choose a reason for hiding this comment

veds-g Jul 7, 2023

Choose a reason for hiding this comment

vigith Jul 6, 2023

Choose a reason for hiding this comment

yhl25 Jul 7, 2023

Choose a reason for hiding this comment

veds-g commented Jul 7, 2023

KeranYang Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

vigith Jul 12, 2023

Choose a reason for hiding this comment

KeranYang Jul 12, 2023

Choose a reason for hiding this comment

whynowy Jul 12, 2023

Choose a reason for hiding this comment

KeranYang Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

yhl25 commented Jun 28, 2023 •

edited by veds-g

Loading

KeranYang Jul 12, 2023 •

edited

Loading

KeranYang Jul 12, 2023 •

edited

Loading