-
Notifications
You must be signed in to change notification settings - Fork 3
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 Runner module #11
Conversation
BREAKING CHANGE: switched exports to use esModuleInterop style
BREAKING CHANGE: all attack interfaces changed
BREAKING CHANGE: runner interface has changed
Object-oriented (classes) and simplify
README.md
Outdated
}, | ||
{ | ||
probability: 0.02, | ||
createAttack: () => { |
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.
Any reason you wanted to expose a function here? No strong reasons against it other than it making me wonder what the purpose is (I'll read through the rest of the PR and it might become apparent!)
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 wanted to allow users to "configure once, run multiple". The runners and attacks are not re-usable - a new instance must be created for each run or attack...
Maybe I've overthought it, and passing the config each time is absolutely fine, but I was trying to support this kind of usage pattern...
const createRunner = Runner.configure({
probability: 1,
possibleAttacks: [
{
probability: 0.01,
createAttack: CPUAttack.configure({ allowLoopEvery: 10 })
},
{
probability: 0.01,
createAttack: OpenFilesAttack.configure({ number: 1000 })
}
]
});
// then, when a request comes in
const runner = createRunner();
runner.start();
// and when the request finishes...
runner.stop();
I think this will also help with the environment configuration (#9 etc), but happy to hear reasons against (complexity etc)
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.
Ah, gotcha, so configure is just a factory function, so they don't need to do that manually.
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 considered calling the configure
methods factory
or something - do you think that'd be a better name?
|
||
const attack = startCPU({ | ||
allowLoopEvery: 1000, | ||
}); |
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.
Any reason to not keep the original (non-runner) example?
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.
Those examples are probably best kept within the individual attacks, WDYT?
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 want to do this ASAP but split into another issue to avoid this from becoming monolithic - #14
README.md
Outdated
import CPUAttack from '@dazn/chaos-squirrel-attack-cpu'; | ||
|
||
const runner = new ChaosRunner({ | ||
// Set a global probability. This defaults to 1, meaning every request is open to chaos |
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.
The "probability" that's mentioned here, what determines that? Is it each invocation of the runner is a roll of the dice?
(As on the other comments, I'll keep digging and I'm sure I'll find the answer 😛 )
Just wondering if we could make it more explicit.
README.md
Outdated
}, | ||
{ | ||
probability: 0.03, | ||
createAttack: () => new CPUAttack({ allowLoopEvery: 100 }) |
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.
Any reason this example uses a constructor, and the other uses the configure method? Is that intentional to show two different API's? Or an error?
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.
Just to show the differences, could use some better documentation
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, might be good just to call that out. I did wonder which was the preferred.
const runner = new ChaosRunner({ | ||
// Set a global probability. This defaults to 1, meaning every request is open to chaos | ||
// Set to 0 to disable all chaos | ||
probability: 1, |
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.
Would be good to mention that this takes precedence over the individually configured probabilities.
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.
Looks good, Simon. 🥳
A couple thoughts from looking through...
- I'm not sure of the benefit of the
createAttack
configuration being a function if it takes no arguments (unless you had future plans for it). - I'm not totally clear on how the running works, should I invoke it once per request, or once per service? It'd be good to clarify this in the README if possible.
- It'd be nice to have an examples folder with the runner working (possibly with some console logs) to show whether it's working or not (that said, I appreciate you mentioned the issue of adding logs)!
Co-authored-by: Lou Bichard <contact@louisjohnbichard.co.uk>
Run attacks at defined random probabilities!
Git diff on this is awful, sorry - it's detected cross-file changes which shouldn't be there... should have used
git mv
instead ofmv
😞Closes #2