fix: load generator improvements #1722
Conversation
Wasn't sanitizing its passed inputs before, which caused a very hard to debug error when deploying to preprod where the RegistryCoordinatorAddr was not populated on the load generator.
Only rebuild when necessary, and make build the default command, as opposed to clean
load generator was crashing when metrics were set to false. Believe this is a mistake in prometheus' promauto API. created an issue for this upstream (see comment in code), and added a hacky fix in our repo in the meantime.
Instead of using one that was read from the config, which was bug prone (and did lead to a bug)
This is needed to debug this call. We had one failing on preprod which was failing with a "execution reverted". We should probably add eth-calls like this before every eth-call site. Unsure if we want info or debug yet...
No longer needed as we read it from the ServiceManager. This is less bug prone.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "SRSPath": "../../../resources/srs", | |||
There was a problem hiding this comment.
Relative paths are evil. Not sure how this was working though... I'm running this from the Makefile in test/v2, so should be ../.. to reach root dir. We should prob use relative paths wrt root though?
There was a problem hiding this comment.
The relative path was correct for running the tests from the IDE (confirmed by trying to run the tests, and they fail).
There was a problem hiding this comment.
I agree this is ugly. I'd have documented this better, but json doesn't allow comments (😠).
I'd prefer to leave the paths so that running the test from an IDE continues to work. It's often quite helpful to run a failing test with a debugger, and it's way more convenient to do that directly from the IDE. The way to get this to also work from the makefile is to run it from the same directory the IDE runs it from.
There was a problem hiding this comment.
Ok think I found the issue and a solution. Turns out golang tests automatically change cwd to be the directory of the test that is currently working. Which is why you guys' ../../../ relative paths were working. However this didn't work with the load-generator because that relative path was interpreted from where the load generator was started.
I changed the paths to be interpreted wrt the config file location, and not the test location. This I argue makes reading the config easier, and also makes using this config file from anywhere work: 59cda27
api/clients/v2/cert_builder.go
Outdated
| nonSignerOperatorIDsHex[i] = "0x" + hex.EncodeToString(id[:]) | ||
| } | ||
| // We log the call parameters for debugging purposes. If execution reverts, we can debug using tenderly. | ||
| cb.logger.Info("eth-call", |
There was a problem hiding this comment.
How would debugging work? Do I have to manually convert to calldata or something like that?
There was a problem hiding this comment.
Idea right now it to use tenderly, which loads the ABI from etherscan and allows to input each field individually.
I would like to also log the calldata directly such that we could also use tenderly or whatever other tool even without the ABI, but that currently doesn't work with the abigen generated code which hides the conversion logic to calldata.
I do plan eventually to switch to abigen v2 which would make this possible though.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "SRSPath": "../../../resources/srs", | |||
There was a problem hiding this comment.
The relative path was correct for running the tests from the IDE (confirmed by trying to run the tests, and they fail).
test/v2/Makefile
Outdated
| rm -rf bin 2>/dev/null || true | ||
|
|
||
| test: | ||
| cd correctness && go test |
There was a problem hiding this comment.
Just a heads up, I'm rebranding this test suite from correctness tests to live tests. Once my branch merges, this will need to change to cd live. I'll keep an eye on this. If your branch merges before mine, I'll fix it before my branch merges.
Before relative paths wrt test files were used in the config file (for SRSPaths), which only worked for the tests, but not for running the load-generator. I made the paths relative to the config file itself, so now they work for both tests and load-generators. I think it also makes understanding the config files easier (otherwise there's some mysterious relative path which doesn't make sense without knowing that the paths are meant to be interpreted wrt where the tests are located).
This way we only log when actually needed (to debug), and don't clutter our happy-path logs for no reason.
Why are these changes needed?
Many small improvements + bug fixes for the load generator.
Please see the commit details for more description on each error/fix.
Checks