-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add rate.Limiter to RunConfiguration #65
base: main
Are you sure you want to change the base?
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 | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -54,6 +54,9 @@ func (g *GenericExecutor) newRun(info ScenarioInfo) (*genericRun, error) { | |||||||||||||||||||||||||||||||||||||
if run.config.MaxConcurrent == 0 { | ||||||||||||||||||||||||||||||||||||||
run.config.MaxConcurrent = g.DefaultConfiguration.MaxConcurrent | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
if run.config.Limiter == nil { | ||||||||||||||||||||||||||||||||||||||
run.config.Limiter = g.DefaultConfiguration.Limiter | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
run.config.ApplyDefaults() | ||||||||||||||||||||||||||||||||||||||
if run.config.Iterations > 0 && run.config.Duration > 0 { | ||||||||||||||||||||||||||||||||||||||
return nil, fmt.Errorf("invalid scenario: iterations and duration are mutually exclusive") | ||||||||||||||||||||||||||||||||||||||
|
@@ -104,8 +107,16 @@ func (g *genericRun) Run(ctx context.Context) error { | |||||||||||||||||||||||||||||||||||||
currentlyRunning++ | ||||||||||||||||||||||||||||||||||||||
run := g.info.NewRun(i + 1) | ||||||||||||||||||||||||||||||||||||||
go func() { | ||||||||||||||||||||||||||||||||||||||
startTime := time.Now() | ||||||||||||||||||||||||||||||||||||||
err := g.executor.Execute(ctx, run) | ||||||||||||||||||||||||||||||||||||||
var runStartTime time.Time | ||||||||||||||||||||||||||||||||||||||
err := func() error { | ||||||||||||||||||||||||||||||||||||||
if g.config.Limiter != nil { | ||||||||||||||||||||||||||||||||||||||
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. Can this be worked into the part above this goroutine where we do waiting? Seeing max-concurrent, we have traditionally waited in the synchronous loop before instantiating the run. |
||||||||||||||||||||||||||||||||||||||
if innerErr := g.config.Limiter.Wait(ctx); innerErr != nil { | ||||||||||||||||||||||||||||||||||||||
return innerErr | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
runStartTime = time.Now() | ||||||||||||||||||||||||||||||||||||||
return g.executor.Execute(ctx, run) | ||||||||||||||||||||||||||||||||||||||
}() | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+110
to
+119
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.
Suggested change
Doesn't seem to be much benefit in the inner function. I don't think we have to overly worry about |
||||||||||||||||||||||||||||||||||||||
// Only log/wrap/send to channel if context is not done | ||||||||||||||||||||||||||||||||||||||
if ctx.Err() == nil { | ||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||
|
@@ -116,7 +127,7 @@ func (g *genericRun) Run(ctx context.Context) error { | |||||||||||||||||||||||||||||||||||||
case <-ctx.Done(): | ||||||||||||||||||||||||||||||||||||||
case doneCh <- err: | ||||||||||||||||||||||||||||||||||||||
// Record/log here, not if it was cut short by context complete | ||||||||||||||||||||||||||||||||||||||
g.executeTimer.Record(time.Since(startTime)) | ||||||||||||||||||||||||||||||||||||||
g.executeTimer.Record(time.Since(runStartTime)) | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
}() | ||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -3,17 +3,18 @@ package loadgen | |||||||
import ( | ||||||||
"context" | ||||||||
"fmt" | ||||||||
"go.temporal.io/api/enums/v1" | ||||||||
"go.temporal.io/api/operatorservice/v1" | ||||||||
"path/filepath" | ||||||||
"runtime" | ||||||||
"strconv" | ||||||||
"strings" | ||||||||
"time" | ||||||||
|
||||||||
"github.com/temporalio/omes/loadgen/kitchensink" | ||||||||
"go.temporal.io/api/enums/v1" | ||||||||
"go.temporal.io/api/operatorservice/v1" | ||||||||
"go.temporal.io/sdk/client" | ||||||||
"go.uber.org/zap" | ||||||||
"golang.org/x/time/rate" | ||||||||
) | ||||||||
|
||||||||
type Scenario struct { | ||||||||
|
@@ -110,6 +111,9 @@ func (s *ScenarioInfo) ScenarioOptionInt(name string, defaultValue int) int { | |||||||
const DefaultIterations = 10 | ||||||||
const DefaultMaxConcurrent = 10 | ||||||||
|
||||||||
// DefaultRateLimiter is unlimited | ||||||||
var DefaultRateLimiter = rate.NewLimiter(rate.Inf, 0) | ||||||||
|
||||||||
Comment on lines
+114
to
+116
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.
Suggested change
Seems a nil limiter is supported at runtime |
||||||||
type RunConfiguration struct { | ||||||||
// Number of iterations to run of this scenario (mutually exclusive with Duration). | ||||||||
Iterations int | ||||||||
|
@@ -119,6 +123,8 @@ type RunConfiguration struct { | |||||||
// Maximum number of instances of the Execute method to run concurrently. | ||||||||
// Default is DefaultMaxConcurrent. | ||||||||
MaxConcurrent int | ||||||||
// Rate limiter to be Wait-ed before each iteration. | ||||||||
Limiter *rate.Limiter | ||||||||
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.
Suggested change
Quite a bit more flexible |
||||||||
} | ||||||||
|
||||||||
func (r *RunConfiguration) ApplyDefaults() { | ||||||||
|
@@ -128,6 +134,9 @@ func (r *RunConfiguration) ApplyDefaults() { | |||||||
if r.MaxConcurrent == 0 { | ||||||||
r.MaxConcurrent = DefaultMaxConcurrent | ||||||||
} | ||||||||
if r.Limiter == nil { | ||||||||
r.Limiter = DefaultRateLimiter | ||||||||
} | ||||||||
Comment on lines
+137
to
+139
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.
Suggested change
Seems a nil rate limiter is supported at runtime |
||||||||
} | ||||||||
|
||||||||
// Run represents an individual scenario run (many may be in a single instance (of possibly many) of a scenario). | ||||||||
|
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.
Seems a nil limiter is supported at runtime