-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,13 +23,14 @@ import ( | |
// CronTracker Track the status of the staging file whether it has reached the terminal state or not for every warehouse | ||
// we pick the staging file which is oldest within the range NOW() - 2 * syncFrequency and NOW() - 3 * syncFrequency | ||
func (r *Router) CronTracker(ctx context.Context) error { | ||
tick := r.statsFactory.NewTaggedStat("warehouse_cron_tracker_tick", stats.CountType, stats.Tags{ | ||
cronTrackerExecTimestamp := r.statsFactory.NewTaggedStat("warehouse_cron_tracker_timestamp_seconds", stats.GaugeType, stats.Tags{ | ||
"module": moduleName, | ||
"destType": r.destType, | ||
}) | ||
for { | ||
|
||
tick.Count(1) | ||
execTime := time.Now() | ||
cronTrackerExecTimestamp.Gauge(execTime.Unix()) | ||
|
||
r.configSubscriberLock.RLock() | ||
warehouses := append([]model.Warehouse{}, r.warehouses...) | ||
|
@@ -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 commentThe 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 commentThe 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. |
||
select { | ||
case <-ctx.Done(): | ||
r.logger.Infon("context is cancelled, stopped running tracking") | ||
return nil | ||
case <-time.After(r.config.uploadStatusTrackFrequency): | ||
case <-time.After(time.Until(nextExecTime)): | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. A more robust approach is to inject 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 |
||
err = r.CronTracker(ctx) | ||
require.NoError(t, err) | ||
|
||
m := statsStore.GetByName("warehouse_cron_tracker_tick") | ||
m := statsStore.GetByName("warehouse_cron_tracker_timestamp_seconds") | ||
require.Equal(t, len(m), 1) | ||
require.Equal(t, m[0].Name, "warehouse_cron_tracker_tick") | ||
require.Equal(t, m[0].Name, "warehouse_cron_tracker_timestamp_seconds") | ||
require.Equal(t, m[0].Tags, stats.Tags{ | ||
"module": moduleName, | ||
"destType": warehouseutils.POSTGRES, | ||
}) | ||
require.GreaterOrEqual(t, m[0].Value, 1.0) | ||
require.GreaterOrEqual(t, m[0].Value, float64(executionTime)) | ||
}) | ||
} |
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.