Conversation
x/cron/keeper/keeper.go
Outdated
| startTime := time.Now() | ||
| schedules := k.getSchedulesReadyForExecution(ctx, executionStage) | ||
|
|
||
| for _, schedule := range schedules { | ||
| err := k.executeSchedule(ctx, schedule) | ||
| recordExecutedSchedule(err, schedule) | ||
| } | ||
| telemetry.ModuleMeasureSince(types.ModuleName, startTime, LabelExecuteReadySchedules) |
There was a problem hiding this comment.
I think just adding defer resolves the problem and it's more cleaner from my perspective (Cosmos SDK uses defer as well):
https://go.dev/play/p/Eof5hpEyxQX
| @@ -205,8 +210,9 @@ func (k *Keeper) executeSchedule(ctx sdk.Context, schedule types.Schedule) error | |||
| ) | |||
| return err | |||
There was a problem hiding this comment.
in case of error, you are missing both telemetry.ModuleMeasureSince calls, it's better to use defer
x/cron/keeper/keeper.go
Outdated
| // and executes messages in each one | ||
| func (k *Keeper) ExecuteReadySchedules(ctx sdk.Context, executionStage types.ExecutionStage) { | ||
| telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), LabelExecuteReadySchedules) | ||
| startTime := time.Now() |
There was a problem hiding this comment.
I still don't understand why do you need a separate variable startTime here.
Just calling an original method telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), LabelExecuteReadySchedules) via defer resolves the initial problem
There was a problem hiding this comment.
Gotcha. Changed
x/cron/keeper/keeper.go
Outdated
| ) | ||
| return err | ||
| } | ||
| defer telemetry.ModuleMeasureSince(types.ModuleName, startTimeContract, LabelExecuteCronContract, schedule.Name, msg.Contract) |
There was a problem hiding this comment.
Hm, suppose we have two schedules:A and B. A gets called first, B is second.
Doesn't this defer call mean, that a measurement for A call contains a time consumption of the B call?
Because the ModuleMeasureSince is called only in the end of the whole execution.
3 participants
fix how timing is reported for cron execution