-
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
[WIP] Add InterruptedIterations metric #1769
Changes from all commits
4448bbf
e2b88ab
645261a
355ee1c
82dae16
ebd2d34
9b47ec8
b0b06c9
c12f449
d52efc4
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 |
---|---|---|
|
@@ -348,7 +348,7 @@ func TestBind(t *testing.T) { | |
}}, | ||
{"Error", bridgeTestErrorType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.error()`) | ||
assert.Contains(t, err.Error(), "GoError: error") | ||
assert.Contains(t, err.Error(), "error") | ||
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 think this can be moved to another PR that we can merge today and make this ... more focused ;) 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. Sure, will do. 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. See #1775. |
||
}}, | ||
{"JSValue", bridgeTestJSValueType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
v, err := RunString(rt, `obj.func(1234)`) | ||
|
@@ -358,7 +358,7 @@ func TestBind(t *testing.T) { | |
}}, | ||
{"JSValueError", bridgeTestJSValueErrorType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.func()`) | ||
assert.Contains(t, err.Error(), "GoError: missing argument") | ||
assert.Contains(t, err.Error(), "missing argument") | ||
|
||
t.Run("Valid", func(t *testing.T) { | ||
v, err := RunString(rt, `obj.func(1234)`) | ||
|
@@ -369,7 +369,7 @@ func TestBind(t *testing.T) { | |
}}, | ||
{"JSValueContext", bridgeTestJSValueContextType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.func()`) | ||
assert.Contains(t, err.Error(), "GoError: func() can only be called from within default()") | ||
assert.Contains(t, err.Error(), "func() can only be called from within default()") | ||
|
||
t.Run("Context", func(t *testing.T) { | ||
*ctxPtr = context.Background() | ||
|
@@ -383,14 +383,14 @@ func TestBind(t *testing.T) { | |
}}, | ||
{"JSValueContextError", bridgeTestJSValueContextErrorType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.func()`) | ||
assert.Contains(t, err.Error(), "GoError: func() can only be called from within default()") | ||
assert.Contains(t, err.Error(), "func() can only be called from within default()") | ||
|
||
t.Run("Context", func(t *testing.T) { | ||
*ctxPtr = context.Background() | ||
defer func() { *ctxPtr = nil }() | ||
|
||
_, err := RunString(rt, `obj.func()`) | ||
assert.Contains(t, err.Error(), "GoError: missing argument") | ||
assert.Contains(t, err.Error(), "missing argument") | ||
|
||
t.Run("Valid", func(t *testing.T) { | ||
v, err := RunString(rt, `obj.func(1234)`) | ||
|
@@ -408,7 +408,7 @@ func TestBind(t *testing.T) { | |
}}, | ||
{"NativeFunctionError", bridgeTestNativeFunctionErrorType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.func()`) | ||
assert.Contains(t, err.Error(), "GoError: missing argument") | ||
assert.Contains(t, err.Error(), "missing argument") | ||
|
||
t.Run("Valid", func(t *testing.T) { | ||
v, err := RunString(rt, `obj.func(1234)`) | ||
|
@@ -419,7 +419,7 @@ func TestBind(t *testing.T) { | |
}}, | ||
{"NativeFunctionContext", bridgeTestNativeFunctionContextType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.func()`) | ||
assert.Contains(t, err.Error(), "GoError: func() can only be called from within default()") | ||
assert.Contains(t, err.Error(), "func() can only be called from within default()") | ||
|
||
t.Run("Context", func(t *testing.T) { | ||
*ctxPtr = context.Background() | ||
|
@@ -433,14 +433,14 @@ func TestBind(t *testing.T) { | |
}}, | ||
{"NativeFunctionContextError", bridgeTestNativeFunctionContextErrorType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.func()`) | ||
assert.Contains(t, err.Error(), "GoError: func() can only be called from within default()") | ||
assert.Contains(t, err.Error(), "func() can only be called from within default()") | ||
|
||
t.Run("Context", func(t *testing.T) { | ||
*ctxPtr = context.Background() | ||
defer func() { *ctxPtr = nil }() | ||
|
||
_, err := RunString(rt, `obj.func()`) | ||
assert.Contains(t, err.Error(), "GoError: missing argument") | ||
assert.Contains(t, err.Error(), "missing argument") | ||
|
||
t.Run("Valid", func(t *testing.T) { | ||
v, err := RunString(rt, `obj.func(1234)`) | ||
|
@@ -464,7 +464,7 @@ func TestBind(t *testing.T) { | |
|
||
t.Run("Negative", func(t *testing.T) { | ||
_, err := RunString(rt, `obj.addWithError(0, -1)`) | ||
assert.Contains(t, err.Error(), "GoError: answer is negative") | ||
assert.Contains(t, err.Error(), "answer is negative") | ||
}) | ||
}}, | ||
{"AddWithError", bridgeTestAddWithErrorType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
|
@@ -475,12 +475,12 @@ func TestBind(t *testing.T) { | |
|
||
t.Run("Negative", func(t *testing.T) { | ||
_, err := RunString(rt, `obj.addWithError(0, -1)`) | ||
assert.Contains(t, err.Error(), "GoError: answer is negative") | ||
assert.Contains(t, err.Error(), "answer is negative") | ||
}) | ||
}}, | ||
{"Context", bridgeTestContextType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.context()`) | ||
assert.Contains(t, err.Error(), "GoError: context() can only be called from within default()") | ||
assert.Contains(t, err.Error(), "context() can only be called from within default()") | ||
|
||
t.Run("Valid", func(t *testing.T) { | ||
*ctxPtr = context.Background() | ||
|
@@ -492,7 +492,7 @@ func TestBind(t *testing.T) { | |
}}, | ||
{"ContextAdd", bridgeTestContextAddType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.contextAdd(1, 2)`) | ||
assert.Contains(t, err.Error(), "GoError: contextAdd() can only be called from within default()") | ||
assert.Contains(t, err.Error(), "contextAdd() can only be called from within default()") | ||
|
||
t.Run("Valid", func(t *testing.T) { | ||
*ctxPtr = context.Background() | ||
|
@@ -506,7 +506,7 @@ func TestBind(t *testing.T) { | |
}}, | ||
{"ContextAddWithError", bridgeTestContextAddWithErrorType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.contextAddWithError(1, 2)`) | ||
assert.Contains(t, err.Error(), "GoError: contextAddWithError() can only be called from within default()") | ||
assert.Contains(t, err.Error(), "contextAddWithError() can only be called from within default()") | ||
|
||
t.Run("Valid", func(t *testing.T) { | ||
*ctxPtr = context.Background() | ||
|
@@ -519,7 +519,7 @@ func TestBind(t *testing.T) { | |
|
||
t.Run("Negative", func(t *testing.T) { | ||
_, err := RunString(rt, `obj.contextAddWithError(0, -1)`) | ||
assert.Contains(t, err.Error(), "GoError: answer is negative") | ||
assert.Contains(t, err.Error(), "answer is negative") | ||
}) | ||
}) | ||
}}, | ||
|
@@ -529,7 +529,7 @@ func TestBind(t *testing.T) { | |
case bridgeTestContextInjectType: | ||
assert.EqualError(t, err, "TypeError: Object has no member 'contextInject' at <eval>:1:18(3)") | ||
case *bridgeTestContextInjectType: | ||
assert.Contains(t, err.Error(), "GoError: contextInject() can only be called from within default()") | ||
assert.Contains(t, err.Error(), "contextInject() can only be called from within default()") | ||
assert.Equal(t, nil, impl.ctx) | ||
|
||
t.Run("Valid", func(t *testing.T) { | ||
|
@@ -588,7 +588,7 @@ func TestBind(t *testing.T) { | |
}}, | ||
{"SumWithContext", bridgeTestSumWithContextType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.sumWithContext(1, 2)`) | ||
assert.Contains(t, err.Error(), "GoError: sumWithContext() can only be called from within default()") | ||
assert.Contains(t, err.Error(), "sumWithContext() can only be called from within default()") | ||
|
||
t.Run("Valid", func(t *testing.T) { | ||
*ctxPtr = context.Background() | ||
|
@@ -626,7 +626,7 @@ func TestBind(t *testing.T) { | |
}}, | ||
{"SumWithContextAndError", bridgeTestSumWithContextAndErrorType{}, func(t *testing.T, obj interface{}, rt *goja.Runtime) { | ||
_, err := RunString(rt, `obj.sumWithContextAndError(1, 2)`) | ||
assert.Contains(t, err.Error(), "GoError: sumWithContextAndError() can only be called from within default()") | ||
assert.Contains(t, err.Error(), "sumWithContextAndError() can only be called from within default()") | ||
|
||
t.Run("Valid", func(t *testing.T) { | ||
*ctxPtr = context.Background() | ||
|
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.
This map lookup is racy according to the current
TestSentReceivedMetrics
failure. So I guess we'll need to rethink or abandon this hack...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 am pretty sure this breaks any output as you are only emitting the
1
to them ...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 it depends on how this metric is agreggated. If their system allows it they could factor it in with the
iterations
metric and get a percentage that way, which is kind of what we're doing here for the summary.It makes no sense to me to emit the
0
value of this metric considering it's equivalent toiterations
, and it would add a considerable amount of output data and affect the performance. A 5s/5VU test with an emptydefault
and no sleep does 2115298 iterations on my machine and generates a 1.5 GB JSON file when emitting the0
value. The same test without it manages to do 3203002 iterations and generate a 1.9 GB JSON file, but this is because it's doing ~50% more iterations.Considering we already have complaints about outputting too much data and interest in #1321, I think we should rather work on #1321 before we consider adding another default metric.
Though we should probably rethink duplicating
iterations
...How about instead of a new metric, we simply reused
iterations
and added aninterrupted: "<cause>"
tag to it? If the tag is omitted it's a complete iteration and if it's set then it was interrupted with the specific cause. WYT?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 agree with prioritizing #1321, but that will actually probably require quite the refactoring, IMO even for an MVP.
The reusage of the iterations was discussed (I think) and we decided against it based on the fact the primary usage for this will be to check that a percentage of the iterations aren't actually interrupted, which with the current thresholds isn't possible if the metric isn't rate... so :(.
I think that maybe we should just make another PR with the changes that are in this PR (the test fixes and the lib.Exception) and merge that to both have them and make this PR easier to rebase and then work on #1321 so we can merge this then.
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.
Then maybe that should be a feature? I'll see what I can do.