Skip to content
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

test: do not run load tests on deployed dev env #1429

Merged
merged 1 commit into from
Mar 25, 2020

Conversation

pgebal
Copy link
Contributor

@pgebal pgebal commented Mar 25, 2020

References: #1428

Overview

As currently tests of load tests do not measure performance drops and are just smoke tests of load test scenarios functionality I removed running them from CI job.

Changes

Removes running smoke load tests on build_test_deploy CI job.

@pgebal pgebal requested a review from kevsul March 25, 2020 09:35
@pgebal
Copy link
Contributor Author

pgebal commented Mar 25, 2020

wait, why?

They just do not test anything regarding deployed env at the moment. We do not collect anything, compare it with prev. runs, etc. No point in running it unless we measure performance on the dev env.

Smoke tests alone are still being run on PRs. But they just test if scenarios themselves are working against the current codebase.

@boolafish
Copy link
Contributor

we do have something. At least you can go to Datadog afterward and see what is there. It can also potentially trigger error to be catch as regression though not on performance but on load. (Though there is no hook back to flag test as failed at this point)

However, my biggest concern is that unless we are sure that we do not want to automate this as a future goal. Otherwise we need step by step code merge anyway. This is discouraging moving that path. Also, it has seen to be a SOP that we want some load in new env from what I see in recent releases/deployment.

@boolafish
Copy link
Contributor

btw, I am looking at the new configs today. So if we want to move forward with this and keep the style of lots of env var. Probably we can just put all configs into one to simplify the configs 🤔

The only reason to have multiple configs was to 1:1 mapping with environments we have, which I think will get a lot more stable after the release process get fixed. I personally still think that makes good sense and fit greatly into a CI/CD pipeline with multiple env.

Meanwhile, the config file could be like ENV_VAR || default_vale so that we don't need to pass the env var in MakeFile. It looks weird to me 🤔. I don't really think that makes much sense actually. (the contract addresses, private key, urls in make file) I think config should be preferred unless you are looking for extra control or the data is confidential.

@pgebal
Copy link
Contributor Author

pgebal commented Mar 25, 2020

Meanwhile, the config file could be like ENV_VAR || default_vale so that we don't need to pass the env var in MakeFile. It looks weird to me thinking. I don't really think that makes much sense actually. (the contract addresses, private key, urls in make file) I think config should be preferred unless you are looking for extra control or the data is confidential.

On one hand it makes a lot of sense and makes things easier for our devs. But on the other, those default values are crafted for us. If people from outside omsisego clone the repo, start running it, those default values probably won't make sense for them.

I agree that putting those env vars in Makefile isn't cool, but I have no good ideas how to handle it differently. Maybe keep scripts that set those vars somewhere in .circleci folder?
Both solutions, keepeing the settings in elixir configs or some other files are problematic as config values need to be updated once new dev env is deployed.

@InoMurko @kevsul what do you think about keeping default contracts addresses in the config? Do you have ideas on how that should be handled now?

@pgebal
Copy link
Contributor Author

pgebal commented Mar 25, 2020

we do have something. At least you can go to Datadog afterward and see what is there. It can also potentially trigger error to be catch as regression though not on performance but on load. (Though there is no hook back to flag test as failed at this point)

I think I get your point. I agree, proper automation of running perfomance test should be done. But I don't think it's now. We have just one smoke test running that puts a couple thousands of utxos into the child chain. We don't know yet
what that tells us about degradation in performance. You are right we can check DataDog, but is anyone doing it? To make it sustainable it should be automated. Now it looks like empty processor cycles :+)

@pgebal
Copy link
Contributor Author

pgebal commented Mar 25, 2020

Also, it has seen to be a SOP that we want some load in new env from what I see in recent releases/deployment.

That makes sense.

@boolafish
Copy link
Contributor

boolafish commented Mar 25, 2020

If people from outside omsisego clone the repo, starts running it, those default values probably won't make sense for them.

I think I messed up a bit on the paragraph so leads a bit confusion. If we are going your way and give up automation until we are perfect, my proposal is having single config with default value to local host (docker-compose). So even an open source contributor can run against the default local service. Whoever wants to run in different environments, pass in the env var.

But honestly I am not really buying "other people might run it" concept personally. I know it is controversial, but practically speaking nobody else is running beside us. And even there are, I don't see reason to not optimize for ourselves. This code is mainly for us anyway. We can make their life easier only after we have made our life easier.

We have just one smoke test running that puts a couple thousands of utxos into the child chain

We do have other scenarios in omg-load-testing not ported yet though.

what that tells us about degradation in performance. You are right we can check DataDog, but is anyone doing it? To make it sustainable it should be automated.

So what is the next step? My feeling is that we would just stop here, write some more scenarios and that is it. It is fine if we agree to do that due to resource/priority reason. But I don't think that is the ideal path.

To make it sustainable it should be automated

probably we could try to call data dog api at the end of the test for two things:

  1. General Error rate. This can potentially be used as an indication to rollback the deployment.
  2. Performance metric.

Even we might not know what is a good threshold yet, it is worth to have report to learn just like the gas cost in contract repo. 🤔

But I guess this depends on what feat-launch-n-load team want to do. cc. @unnawut @kevsul

@pgebal pgebal merged commit 3583d80 into master Mar 25, 2020
@pgebal pgebal deleted the pgebal/remove-load-test-on-deployment-ci-job branch March 25, 2020 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants