Feature/169366221 flux monitor heartbeat by felder-cl · Pull Request #2233 · smartcontractkit/chainlink

felder-cl · 2020-01-30T16:41:57Z

Allow chainlink operators to configure the Flux Monitor to trigger a Job Run if it has exceeded a specified 'idle time'. The 'idle time' is the time for which no Job Run has been started.

The behavior is disabled if no idle time is defined.

https://www.pivotaltracker.com/story/show/169366221

samsondav · 2020-02-07T13:34:39Z

Any reason for this choice of default? Seems arbitrary.

True, I've updated it to be greater than or equal to the pollingInterval

rupurt · 2020-02-12T03:09:05Z

+	cancel        context.CancelFunc
+	newRounds     chan eth.Log
+
+	stopC chan struct{}


What's the significance of C in stopC? Is it indicating Checker or Channel?

Its for channel, would you prefer another name or have any suggestions?

I like the change to waitOnStop as we can lookup that it's channel from the type. Captures the intent well.

rupurt · 2020-02-12T03:17:06Z

 		{"inside deviation", decimal.NewFromInt(100), decimal.NewFromInt(101), 2, false},
 		{"equal to deviation", decimal.NewFromInt(100), decimal.NewFromInt(102), 2, true},
 		{"outside deviation", decimal.NewFromInt(100), decimal.NewFromInt(103), 2, true},
+		{"outside deviation zero", decimal.NewFromInt(100), decimal.NewFromInt(0), 2, true},


This is perhaps outside the scope of this PR...

But how do you feel about adding test coverage for when threshold is negative?

The difference between current price and next price is computed as an absolute value... But if curPrice could go negative, OutsideDeviation would not trigger. I don't know whether there's a check elsewhere in the code that the price is non-negative. Decimals can be negative.

i feel validation and these constraints are outside the scope for sure. It probably deserves its own story and we are waiting to hear back re: the audit.

coventry

Overall, this looks good, but there are some aspects to it I don't follow, and I think it would probably be helpful to other readers if you could clarify them in the code.

coventry · 2020-02-12T02:41:55Z

+func stopTimer(arg *time.Timer) {
+	if !arg.Stop() && len(arg.C) > 0 {
+		// timer's channel may have backlog
+		// refer to: https://developpaper.com/detailed-explanation-of-the-trap-of-timer-in-golang/


Is there a more authoritative reference for this? The writing on that site is a bit difficult to follow.

The point is to drain the potential backlog? What could go wrong if you didn't?

Authoritative yes, better explained, no. This was the best article I could find on the subject.

I've updated the comment, but I've assumed the reader has an understanding of the operation of the timer entity. Therefor I've left out what could go wrong.

a201e38

coventry · 2020-02-12T05:36:54Z

 func (p *PollingDeviationChecker) Stop() {
 	if p.cancel != nil {
 		p.cancel()
+		<-p.stopC


I'm unsure about the purpose of this. Is it to hang until the channel is closed in the consumer loop?

I think an explanatory comment is helpful, when this kind of action at a distance is going on.

I've renamed it to waitOnStop

coventry · 2020-02-12T05:44:17Z

+	err = deviationChecker.Start(context.Background(), ethClient)
+	require.NoError(t, err)
+
+	<-fetchCalled // Called on start


This will hang on failure, which is a bit inconvenient. Would it be straightforward to wrap this in a timeout shorter than 10 minutes?

Thanks for pointing this out

I've improved the test a bit and fixed this problem in 8d49a90cc551dca77da8f13358ba71076a15482a

coventry · 2020-02-12T05:49:19Z

 		{"inside deviation", decimal.NewFromInt(100), decimal.NewFromInt(101), 2, false},
 		{"equal to deviation", decimal.NewFromInt(100), decimal.NewFromInt(102), 2, true},
 		{"outside deviation", decimal.NewFromInt(100), decimal.NewFromInt(103), 2, true},
+		{"outside deviation zero", decimal.NewFromInt(100), decimal.NewFromInt(0), 2, true},


The difference between current price and next price is computed as an absolute value... But if curPrice could go negative, OutsideDeviation would not trigger. I don't know whether there's a check elsewhere in the code that the price is non-negative. Decimals can be negative.

thodges-gh · 2020-02-15T14:58:42Z

It doesn't seem like this new "idleThreshold" is actually working.

felder-cl · 2020-02-18T15:37:18Z

It doesn't seem like this new "idleThreshold" is actually working.

There was a logic error where the idleThreshold was linked to the last time the price was polled, not updated. This is fixed in cdbdca3.

Thanks for spotting this.

Allow chainlink operators to configure the Flux Monitor to trigger a Job Run if it has exceeded a specified 'idle time'. The 'idle time' is the time for which no Job Run has been started. The behavior is disabled if no IdleTime is defined. https://www.pivotaltracker.com/story/show/169366221 To make testing easier, ensure the DeviationChecker has stopped completely before returning.

Fix a logic error the implementation of the Idle Threshold by making the timeout occur since the last Job Run. The IdleThreshold is mistakenly linked to the last pollingInterval check.

Resolved.

felder-cl force-pushed the feature/169366221-flux-monitor-heartbeat branch 2 times, most recently from 0a8a2ac to 57c7472 Compare February 5, 2020 15:29

felder-cl requested a review from thodges-gh February 5, 2020 15:29

felder-cl force-pushed the feature/169366221-flux-monitor-heartbeat branch 6 times, most recently from e41ce80 to 69a1ae6 Compare February 7, 2020 12:07

felder-cl mentioned this pull request Feb 7, 2020

FluxMonitor: Expose DeviationChecker concurrency #2259

Closed

felder-cl requested review from samsondav, se3000 and spooktheducks February 7, 2020 12:12

samsondav reviewed Feb 7, 2020

View reviewed changes

felder-cl force-pushed the feature/169366221-flux-monitor-heartbeat branch 2 times, most recently from 9f6bfb3 to f340be9 Compare February 7, 2020 14:46

felder-cl marked this pull request as ready for review February 7, 2020 14:58

felder-cl requested a review from samsondav February 7, 2020 16:16

felder-cl marked this pull request as ready for review February 7, 2020 16:16

samsondav reviewed Feb 11, 2020

View reviewed changes

Comment thread core/services/flux_monitor_helpers_test.go

samsondav reviewed Feb 11, 2020

View reviewed changes

Comment thread core/web/job_specs_controller_test.go Outdated

rupurt reviewed Feb 12, 2020

View reviewed changes

coventry previously requested changes Feb 12, 2020

View reviewed changes

felder-cl force-pushed the feature/169366221-flux-monitor-heartbeat branch 3 times, most recently from 8d49a90 to 34f4e1c Compare February 12, 2020 15:17

felder-cl requested review from coventry, rupurt and samsondav February 12, 2020 15:44

samsondav previously approved these changes Feb 12, 2020

View reviewed changes

felder-cl dismissed stale reviews from thodges-gh and samsondav via a8c55ae February 14, 2020 12:43

felder-cl force-pushed the feature/169366221-flux-monitor-heartbeat branch from 34f4e1c to a8c55ae Compare February 14, 2020 12:43

felder-cl force-pushed the feature/169366221-flux-monitor-heartbeat branch from a8c55ae to 9107e42 Compare February 17, 2020 13:49

smartcontractkit deleted a comment Feb 17, 2020

felder-cl force-pushed the feature/169366221-flux-monitor-heartbeat branch 7 times, most recently from 9f92b04 to 0057e68 Compare February 18, 2020 15:32

felder-cl force-pushed the feature/169366221-flux-monitor-heartbeat branch 2 times, most recently from 660de6f to 804dd45 Compare February 18, 2020 17:36

thodges-gh previously approved these changes Feb 18, 2020

View reviewed changes

felder-cl added 7 commits February 18, 2020 19:25

flux monitor integration: Archive job end of test

8d79707

Flux Monitor: Tie idleThreshold to last Job Run

dddc615

Fix a logic error the implementation of the Idle Threshold by making the timeout occur since the last Job Run. The IdleThreshold is mistakenly linked to the last pollingInterval check.

remove unreachable code

5229041

integration test: fix sleep in fm test

6d089cd

make comment a message

042fbb1

Improve Job Spec controller test

7709d56

felder-cl dismissed thodges-gh’s stale review via 7709d56 February 18, 2020 18:25

felder-cl force-pushed the feature/169366221-flux-monitor-heartbeat branch from 804dd45 to 7709d56 Compare February 18, 2020 18:25

thodges-gh approved these changes Feb 18, 2020

View reviewed changes

thodges-gh merged commit ba9b253 into develop Feb 18, 2020

j16r deleted the feature/169366221-flux-monitor-heartbeat branch March 3, 2020 20:36

Uh oh!

Conversation

felder-cl commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rupurt Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coventry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felder-cl Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felder-cl Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thodges-gh commented Feb 15, 2020

Uh oh!

felder-cl commented Feb 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

felder-cl commented Jan 30, 2020 •

edited

Loading

rupurt Feb 12, 2020 •

edited

Loading

felder-cl Feb 12, 2020 •

edited

Loading

felder-cl Feb 12, 2020 •

edited

Loading