-
Notifications
You must be signed in to change notification settings - Fork 314
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.
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
fix: replace counter with gauge cron tracker alert #5084
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
counter resetting on restarts was causing false alerts
02cff1b
to
8fb3325
Compare
Once in a while, cron runtime may be delayed due to various reasons. This commit disregards the cron runtime in scheduling the next run.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5084 +/- ##
==========================================
+ Coverage 74.65% 74.74% +0.09%
==========================================
Files 435 435
Lines 50378 50386 +8
==========================================
+ Hits 37608 37663 +55
+ Misses 10286 10255 -31
+ Partials 2484 2468 -16 ☔ View full report in Codecov by Sentry. |
"module": moduleName, | ||
"destType": r.destType, | ||
}) | ||
for { | ||
|
||
tick.Count(1) | ||
execTime := time.Now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
execTime := time.Now() | |
execTime := r.Now() |
@@ -215,16 +215,17 @@ func TestRouter_CronTracker(t *testing.T) { | |||
|
|||
mockLogger.EXPECT().Infon("context is cancelled, stopped running tracking").Times(1) | |||
|
|||
executionTime := time.Now().Unix() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more robust approach is to inject time.Now
into the router.
now := time.Now()
r := Router{
now: func() time.Time{
return now
}
}
In this particular case, you are calling time.Now() immediately. So observing an issue would be rare
@@ -51,11 +52,12 @@ func (r *Router) CronTracker(ctx context.Context) error { | |||
} | |||
} | |||
|
|||
nextExecTime := execTime.Add(r.config.uploadStatusTrackFrequency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea, but we might constantly query the database if the execution time is high.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tweaking cron frequency should help us reduce load if/when needed. I will also look into improving the execution time, if any possible.
Description
counter resetting on restarts was causing false alerts. As the operation being tracked (cron tracker execution) doesn't happen often, using epoch gauge metric will help.
Linear Ticket
part of PIPE-1514
Security