-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Summary callback changes #1768
Summary callback changes #1768
Changes from 3 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 |
---|---|---|
|
@@ -26,6 +26,7 @@ import ( | |
"crypto/tls" | ||
"encoding/json" | ||
"fmt" | ||
"io" | ||
"net" | ||
"net/http" | ||
"net/http/cookiejar" | ||
|
@@ -244,10 +245,7 @@ func (r *Runner) newVU(id int64, samplesOut chan<- stats.SampleContainer) (*VU, | |
} | ||
|
||
func (r *Runner) Setup(ctx context.Context, out chan<- stats.SampleContainer) error { | ||
setupCtx, setupCancel := context.WithTimeout( | ||
ctx, | ||
time.Duration(r.Bundle.Options.SetupTimeout.Duration), | ||
) | ||
setupCtx, setupCancel := context.WithTimeout(ctx, r.getTimeoutFor(consts.SetupFn)) | ||
defer setupCancel() | ||
|
||
v, err := r.runPart(setupCtx, out, consts.SetupFn, nil) | ||
|
@@ -279,10 +277,7 @@ func (r *Runner) SetSetupData(data []byte) { | |
} | ||
|
||
func (r *Runner) Teardown(ctx context.Context, out chan<- stats.SampleContainer) error { | ||
teardownCtx, teardownCancel := context.WithTimeout( | ||
ctx, | ||
time.Duration(r.Bundle.Options.TeardownTimeout.Duration), | ||
) | ||
teardownCtx, teardownCancel := context.WithTimeout(ctx, r.getTimeoutFor(consts.TeardownFn)) | ||
defer teardownCancel() | ||
|
||
var data interface{} | ||
|
@@ -312,6 +307,77 @@ func (r *Runner) IsExecutable(name string) bool { | |
return exists | ||
} | ||
|
||
// HandleSummary calls the specified summary callback, if supplied. | ||
func (r *Runner) HandleSummary(ctx context.Context, summary *lib.Summary) (map[string]io.Reader, error) { | ||
summaryDataForJS := summarizeMetricsToObject(summary, r.Bundle.Options) | ||
|
||
out := make(chan stats.SampleContainer, 100) | ||
defer close(out) | ||
|
||
go func() { // discard all metrics | ||
for range out { | ||
} | ||
}() | ||
|
||
vu, err := r.newVU(0, out) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
handleSummaryFn := goja.Undefined() | ||
if exported := vu.Runtime.Get("exports").ToObject(vu.Runtime); exported != nil { | ||
fn := exported.Get(consts.HandleSummaryFn) | ||
if _, ok := goja.AssertFunction(fn); ok { | ||
imiric marked this conversation as resolved.
Show resolved
Hide resolved
|
||
handleSummaryFn = fn | ||
} else if fn != nil && !goja.IsUndefined(fn) && !goja.IsNull(fn) { | ||
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. I checked and I am not particularly certain it matters either way, to be honest. If someone has defined 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.
Likely that, and it makes sense to leave only the |
||
return nil, fmt.Errorf("exported identfier %s must be a function", consts.HandleSummaryFn) | ||
} | ||
} | ||
ctx = common.WithRuntime(ctx, vu.Runtime) | ||
ctx = lib.WithState(ctx, vu.state) | ||
ctx, cancel := context.WithTimeout(ctx, r.getTimeoutFor(consts.HandleSummaryFn)) | ||
defer cancel() | ||
go func() { | ||
<-ctx.Done() | ||
vu.Runtime.Interrupt(context.Canceled) | ||
}() | ||
*vu.Context = ctx | ||
|
||
handleSummaryWrapperRaw, err := vu.Runtime.RunString(summaryWrapperLambdaCode) | ||
if err != nil { | ||
return nil, fmt.Errorf("unexpected error while getting the summary wrapper: %w", err) | ||
} | ||
handleSummaryWrapper, ok := goja.AssertFunction(handleSummaryWrapperRaw) | ||
if !ok { | ||
return nil, fmt.Errorf("unexpected error did not get a callable summary wrapper") | ||
} | ||
|
||
wrapperArgs := []goja.Value{ | ||
handleSummaryFn, | ||
vu.Runtime.ToValue(r.Bundle.RuntimeOptions.SummaryExport.String), | ||
vu.Runtime.ToValue(summaryDataForJS), | ||
vu.Runtime.ToValue(getOldTextSummaryFunc(summary, r.Bundle.Options)), // TODO: remove | ||
} | ||
rawResult, _, _, err := vu.runFn(ctx, false, handleSummaryWrapper, wrapperArgs...) | ||
|
||
// TODO: refactor the whole JS runner to avoid copy-pasting these complicated bits... | ||
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. I am pretty sure you can just use 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. Although I guess you will need to move https://github.com/loadimpact/k6/blob/0838813db6edead0f6faa3cdaadf3b5eb070a1ab/js/summary.go#L95-L118 to golang code after the 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. Yeah, I initially tried using |
||
// deadline is reached so we have timeouted but this might've not been registered correctly | ||
if deadline, ok := ctx.Deadline(); ok && time.Now().After(deadline) { | ||
// we could have an error that is not context.Canceled in which case we should return it instead | ||
if err, ok := err.(*goja.InterruptedError); ok && rawResult != nil && err.Value() != context.Canceled { | ||
// TODO: silence this error? | ||
return nil, err | ||
} | ||
// otherwise we have timeouted | ||
return nil, lib.NewTimeoutError(consts.HandleSummaryFn, r.getTimeoutFor(consts.HandleSummaryFn)) | ||
} | ||
|
||
if err != nil { | ||
return nil, fmt.Errorf("unexpected error while generating the summary: %w", err) | ||
} | ||
return getSummaryResult(rawResult) | ||
} | ||
|
||
func (r *Runner) SetOptions(opts lib.Options) error { | ||
r.Bundle.Options = opts | ||
r.RPSLimit = nil | ||
|
@@ -434,19 +500,21 @@ func (r *Runner) runPart(ctx context.Context, out chan<- stats.SampleContainer, | |
return v, err | ||
} | ||
// otherwise we have timeouted | ||
return v, lib.NewTimeoutError(name, r.timeoutErrorDuration(name)) | ||
return v, lib.NewTimeoutError(name, r.getTimeoutFor(name)) | ||
} | ||
return v, err | ||
} | ||
|
||
// timeoutErrorDuration returns the timeout duration for given stage. | ||
func (r *Runner) timeoutErrorDuration(stage string) time.Duration { | ||
// getTimeoutFor returns the timeout duration for given special script function. | ||
func (r *Runner) getTimeoutFor(stage string) time.Duration { | ||
d := time.Duration(0) | ||
switch stage { | ||
case consts.SetupFn: | ||
return time.Duration(r.Bundle.Options.SetupTimeout.Duration) | ||
case consts.TeardownFn: | ||
return time.Duration(r.Bundle.Options.TeardownTimeout.Duration) | ||
case consts.HandleSummaryFn: | ||
return 2 * time.Minute // TODO: make configurable | ||
} | ||
return d | ||
} | ||
|
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.
I guess that in a sequential release we will fallback to a default representation or the dumping the
lib.Summary
as JSON to the disk?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.
not sure what you mean by "a sequential release", but we'll get an error here only if something goes majorly wrong... Both
initRunner.HandleSummary()
andhandleSummaryResult()
try to continue working even if there are errors.handleSummaryResult()
tries to save all files, even if some of them fail (because of wrong names, lack of permissions, etc.). AndinitRunner.HandleSummary()
has internaltry / catch
in the JS wrapper code: https://github.com/loadimpact/k6/blob/0838813db6edead0f6faa3cdaadf3b5eb070a1ab/js/summary.go#L97-L104Though I should add tests for both of these things...
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.
yeah, I missed this code at the time ...
This is probably my biggest issue reviewing the PR, the code is all over the place, although I don't know how to fix that if it's at all possible given what the code tries to do, so I don't have some concrete feedback. I hope that when we rewrite even more of the whole metric and threshold (which specifically are also over the place) this might get more streamlined