Skip to content
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions x/cron/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (

var (
LabelExecuteReadySchedules = "execute_ready_schedules"
LabelExecuteCronSchedule = "execute_cron_schedule"
LabelExecuteCronContract = "execute_cron_contract"
LabelScheduleCount = "schedule_count"
LabelScheduleExecutionsCount = "schedule_executions_count"

Expand Down Expand Up @@ -67,13 +69,14 @@ func (k *Keeper) Logger(ctx sdk.Context) log.Logger {
// ExecuteReadySchedules gets all schedules that are due for execution (with limit that is equal to Params.Limit)
// 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()
Copy link
Collaborator

@pr0n00gler pr0n00gler Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Changed

schedules := k.getSchedulesReadyForExecution(ctx, executionStage)

for _, schedule := range schedules {
err := k.executeSchedule(ctx, schedule)
recordExecutedSchedule(err, schedule)
}
telemetry.ModuleMeasureSince(types.ModuleName, startTime, LabelExecuteReadySchedules)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// AddSchedule adds a new schedule to be executed every certain number of blocks, specified in the `period`.
Expand Down Expand Up @@ -182,12 +185,14 @@ func (k *Keeper) getSchedulesReadyForExecution(ctx sdk.Context, executionStage t
func (k *Keeper) executeSchedule(ctx sdk.Context, schedule types.Schedule) error {
// Even if contract execution returned an error, we still increase the height
// and execute it after this interval
startTimeSchedule := time.Now()
schedule.LastExecuteHeight = uint64(ctx.BlockHeight()) //nolint:gosec
k.storeSchedule(ctx, schedule)

cacheCtx, writeFn := ctx.CacheContext()

for idx, msg := range schedule.Msgs {
startTimeContract := time.Now()
executeMsg := wasmtypes.MsgExecuteContract{
Sender: k.accountKeeper.GetModuleAddress(types.ModuleName).String(),
Contract: msg.Contract,
Expand All @@ -205,8 +210,9 @@ func (k *Keeper) executeSchedule(ctx sdk.Context, schedule types.Schedule) error
)
return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in case of error, you are missing both telemetry.ModuleMeasureSince calls, it's better to use defer

}
telemetry.ModuleMeasureSince(types.ModuleName, startTimeContract, LabelExecuteCronContract, schedule.Name, msg.Contract)
}

telemetry.ModuleMeasureSince(types.ModuleName, startTimeSchedule, LabelExecuteCronSchedule, schedule.Name)
// only save state if all the messages in a schedule were executed successfully
writeFn()
return nil
Expand Down
Loading