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

Setup for remote relationship benchmarks using sportsdb #70

Open
wants to merge 7 commits into
base: add-remote-relationship-no-op
Choose a base branch
from

Conversation

nizar-m
Copy link
Collaborator

@nizar-m nizar-m commented Oct 9, 2019

Description

The python script test_with_sportsdb.py script should setup the databases and graphql engines required for the tests.

Affected components

  • Benchmarks

Related Issues

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

@nizar-m nizar-m force-pushed the remote-relationshps-benchmarks branch from 4895c23 to fbb8263 Compare October 10, 2019 03:58
Copy link
Collaborator

@jberryman jberryman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I run the final benchmarking step I get this:

====================
benchmark: events_remote_affilications
  --------------------
  candidate: events_remote_affiliations on hge-with-remote at http://127.0.0.1:8081/v1/graphql
    Warmup:
      ++++++++++++++++++++
      20Req/s Duration:60s open connections:20
      unable to connect to 127.0.0.1:8081 Connection refused
      ++++++++++++++++++++
      40Req/s Duration:60s open connections:20
      unable to connect to 127.0.0.1:8081 Connection refused
    Benchmark:
      ++++++++++++++++++++
      20Req/s Duration:300s open connections:20
      unable to connect to 127.0.0.1:8081 Connection refused
      ++++++++++++++++++++
      40Req/s Duration:300s open connections:20
      unable to connect to 127.0.0.1:8081 Connection refused
 * Serving Flask app "bench" (lazy loading)
 * Environment: production
   WARNING: Do not use the development server in a production environment.
   Use a production WSGI server instead.
 * Debug mode: off
 * Running on http://0.0.0.0:8050/ (Press CTRL+C to quit)

And visiting http://0.0.0.0:8050/ shows an empty graph

The Python script `test_with_sportsdb.py` will help in setting up the databases, starting the Hasura GraphQL engines, and setting up relationships (both local and remote). This script will run databases on docker, and the GraphQL engines are run with `stack exec`.

#### Setup GraphQL Engines ####

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to add instructions for installing dependencies in a virtual environment:

$ python3 -m venv venv                                                                                                                                                                                                                     
$ source venv/bin/activate
$ pip3 install -r requirements.txt

and also to add that venv directory to the gitignore (there is probably a better name than "venv").

Also for some reason when I do the above I get some error messages:

Building wheels for collected packages: psycopg2, inflection
  Running setup.py bdist_wheel for psycopg2 ... error
  Complete output from command /home/me/Work/hasura/graphql-engine/server/tests-py/remote_relationship_tests/venv/bin/python3 -u -c "import setuptools, tokenize;__file__='/tmp/pip-build-bfabn9ae/psycopg2/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" bdist_wheel -d /tmp/tmpdkk7678ipip-wheel- --python-tag cp35:
  /usr/lib/python3.5/distutils/dist.py:261: UserWarning: Unknown distribution option: 'project_urls'
    warnings.warn(msg)
  usage: -c [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
     or: -c --help [cmd1 cmd2 ...]
     or: -c --help-commands
     or: -c cmd --help
  
  error: invalid command 'bdist_wheel'
  
  ----------------------------------------
  Failed building wheel for psycopg2
  Running setup.py clean for psycopg2
  Running setup.py bdist_wheel for inflection ... error
  Complete output from command /home/me/Work/hasura/graphql-engine/server/tests-py/remote_relationship_tests/venv/bin/python3 -u -c "import setuptools, tokenize;__file__='/tmp/pip-build-bfabn9ae/inflection/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" bdist_wheel -d /tmp/tmpez0s8qnepip-wheel- --python-tag cp35:
  usage: -c [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
     or: -c --help [cmd1 cmd2 ...]
     or: -c --help-commands
     or: -c cmd --help
  
  error: invalid command 'bdist_wheel'
  
  ----------------------------------------
  Failed building wheel for inflection
  Running setup.py clean for inflection

...but the overall pip3 command returns 0 status and the script seemed to start fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified readme to add the instructions for installing dependencies in a virtual environment

I did not face the bdist_wheel error. Probably because I am using Python 3.7

if self.admin_secret:
hge_env['HASURA_GRAPHQL_ADMIN_SECRET'] = self.admin_secret

process_args = ['stack', 'exec', 'graphql-engine', '--', 'serve']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should do a stack build first by default here. Otherwise the user is going to benchmark the wrong code by accident and draw incorrect conclusions.

In dev.sh I do this : https://github.com/hasura/graphql-engine/blob/master/scripts/dev.sh#L130 . Obviously we don't want to build using --fast for benchmarks. I think just stack build is probably right.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Made changes to do stack build before running the graphql engines.


To run the benchmark, do
```sh
cat bench.yaml | docker run -i --rm -p 8050:8050 -v $(pwd)/queries.graphql:/graphql-bench/ws/queries.graphql hasura/graphql-bench:v0.3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be convenient if this bench,yaml or a variation that was a good default already existed somewhere, checked in to git.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example benchmark file is included in the directory (example-bench.yaml)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to remove the inline yaml from the Readme.md and just say "you can copy the example bench.yaml included at , and then run..."

@jberryman
Copy link
Collaborator

It would be great if we could compare performance against master for queries that don't involve remote joins. What would be the best way to accomplish that? Can this PR work against master?


To run the benchmark, do
```sh
cat bench.yaml | docker run -i --rm -p 8050:8050 -v $(pwd)/queries.graphql:/graphql-bench/ws/queries.graphql hasura/graphql-bench:v0.3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to understand what this is doing a little better. e.g. are we testing throughput? latency at different concurrency levels? If I run this script on a two-core machine will I receive sensible output? etc.

I'm sure some of this would be obvious if I got successful output. But maybe there are some docs that could be linked here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is mainly testing the latency, the average, variance, 95 percentile etc. We can vary the number of request per second though.

@jberryman
Copy link
Collaborator

Ah I also noticed on my machine this creates directories that have weird owenership:

total 16552
-rw-r--r--  1 me me       2797 Oct 10 12:15 hge.log
-rw-r--r--  1 me me       3181 Oct 10 12:15 remote_hge.log
drwx------ 19 70 root     4096 Oct 10 12:15 remote_sportsdb_data
-rw-r--r--  1 me me     675840 Oct 10 11:32 sportsdb_cache.sqlite
drwx------ 19 70 root     4096 Oct 10 12:15 sportsdb_data
-rw-r--r--  1 me me   15597968 Oct 10 11:32 sportsdb_sample_postgresql_20080304.sql
-rw-r--r--  1 me me     652509 Oct 10 11:32 sportsdb.zip

This causes problems for tooling like hasktags or ack, since they can't be traversed.

@nizar-m
Copy link
Collaborator Author

nizar-m commented Oct 11, 2019

When I run the final benchmarking step I get this:

====================
benchmark: events_remote_affilications
  --------------------
  candidate: events_remote_affiliations on hge-with-remote at http://127.0.0.1:8081/v1/graphql
    Warmup:
      ++++++++++++++++++++
      20Req/s Duration:60s open connections:20
      unable to connect to 127.0.0.1:8081 Connection refused
      ++++++++++++++++++++
      40Req/s Duration:60s open connections:20
      unable to connect to 127.0.0.1:8081 Connection refused
    Benchmark:
      ++++++++++++++++++++
      20Req/s Duration:300s open connections:20
      unable to connect to 127.0.0.1:8081 Connection refused
      ++++++++++++++++++++
      40Req/s Duration:300s open connections:20
      unable to connect to 127.0.0.1:8081 Connection refused
 * Serving Flask app "bench" (lazy loading)
 * Environment: production
   WARNING: Do not use the development server in a production environment.
   Use a production WSGI server instead.
 * Debug mode: off
 * Running on http://0.0.0.0:8050/ (Press CTRL+C to quit)

And visiting http://0.0.0.0:8050/ shows an empty graph

The connection from docker to a localhost application will work only when the docker is running with --net=host. I have changed the readme to reflect this. (This may not work on mac though).

@nizar-m
Copy link
Collaborator Author

nizar-m commented Oct 11, 2019

Ah I also noticed on my machine this creates directories that have weird owenership:

total 16552
-rw-r--r--  1 me me       2797 Oct 10 12:15 hge.log
-rw-r--r--  1 me me       3181 Oct 10 12:15 remote_hge.log
drwx------ 19 70 root     4096 Oct 10 12:15 remote_sportsdb_data
-rw-r--r--  1 me me     675840 Oct 10 11:32 sportsdb_cache.sqlite
drwx------ 19 70 root     4096 Oct 10 12:15 sportsdb_data
-rw-r--r--  1 me me   15597968 Oct 10 11:32 sportsdb_sample_postgresql_20080304.sql
-rw-r--r--  1 me me     652509 Oct 10 11:32 sportsdb.zip

This causes problems for tooling like hasktags or ack, since they can't be traversed.

You can now specify the directory where all these files should be present. If you use the same work directory the second time, bringing up the test setup would be much faster. Instead of doing the full setup, we would simple reuse the Postgres data directories sportsdb_data and remote_sportsdb_data (these directories are bind mounted to Postgres dockers).

@nizar-m
Copy link
Collaborator Author

nizar-m commented Oct 11, 2019

Probably we can run the master using the corresponding docker image, and then run benchmarks.

So I guess we need to make the following comparisions:

Master vs remote relationship branch for the normal queries

Query with table object/array relationship vs with remote object/array relationship

Do we have other comparisons to make?

@jberryman
Copy link
Collaborator

Ah I also noticed on my machine this creates directories that have weird owenership:

total 16552
-rw-r--r--  1 me me       2797 Oct 10 12:15 hge.log
-rw-r--r--  1 me me       3181 Oct 10 12:15 remote_hge.log
drwx------ 19 70 root     4096 Oct 10 12:15 remote_sportsdb_data
-rw-r--r--  1 me me     675840 Oct 10 11:32 sportsdb_cache.sqlite
drwx------ 19 70 root     4096 Oct 10 12:15 sportsdb_data
-rw-r--r--  1 me me   15597968 Oct 10 11:32 sportsdb_sample_postgresql_20080304.sql
-rw-r--r--  1 me me     652509 Oct 10 11:32 sportsdb.zip

This causes problems for tooling like hasktags or ack, since they can't be traversed.

You can now specify the directory where all these files should be present. If you use the same work directory the second time, bringing up the test setup would be much faster. Instead of doing the full setup, we would simple reuse the Postgres data directories sportsdb_data and remote_sportsdb_data (these directories are bind mounted to Postgres dockers).

That's an okay workaround. It would be nicer if they lived in the tree but not owned by root. But it looks like this is a pain to do; not sure:

https://gist.github.com/nitrobin/4d16fbe347c150a422ad
moby/moby#2259

@jberryman
Copy link
Collaborator

jberryman commented Oct 21, 2019

I was able to try again and everything went smoothly I think, following your instructions!

Random thoughts mostly for when I get a chance to integrate this with my dev.sh script, and mostly concerning improvements to https://github.com/hasura/graphql-bench

  • I'm not sure why we need to run a warmup for every rps setting...
  • errors or timeouts will be super important to get in front of eyes somehow, either in a summary or on a graph somehow... EDIT: ya, now that I'm digging in I don't see that errors are surfaced in any way (I suppose this is first and foremost an issue with wrk or how we're using it); it would be good to have the ability to benchmark non-200 queries, but not when we're expecting successful responses. We especially want to be able to see when higher load is breaking things. Honestly I'd rather see every sample on some kind of scatter plot, with failures marked in red and trend lines for percentiles
  • We'll probably want a flag that will allow bypassing the stack build (since we may be building with profiling, etc.)
  • when running multiple candidates it looks like some get dropped from the graph at higher RPS, though I don't see obvious errors reported
  • you can't look at plots for two different runs at the same time in different browser tabs; since switching "response time metric" will just pull data from most recent run
  • We should be able to see at least P50/P95/P90 at a glance all in the same graph;
    • also if there is a way to snap the graph height to e.g. a power of 2, this makes it much easier to sort of eyeball and compare runs
    • also I don't know if the x axis needs to be a scale... I think we can just label several distinct plots and put them side by side
  • we can think more carefully about what questions we're really trying to answer and script for that; e.g. we might like to know how many requests per second can we support before latency starts to degrade? (we can figure this out pretty easily using some trial and error, which is great)
  • my preferred way to compare benchmark metrics (e.g. comparing performance of two or more branches) is to have a table of percent changes that includes coloring indicating how good or bad the change was, and how significant. (I think this may only make sense when comparing the mean). e.g.:

bench

@jberryman
Copy link
Collaborator

Would you mind adding a link to the included queries.graphql in the readme, and mentioning how one selects the queries with query: in the YAML? It's pretty self-explanatory once you know what files to look at but would be helpful. Maybe this belongs in the docs for https://github.com/hasura/graphql-bench instead

@jberryman
Copy link
Collaborator

Also it would be really awesome if you could include a bunch of interesting queries in queries.graphql with comments. e.g. it would be nice to have some that:

  • return a lot of data but with simple SQL
  • return a small amount of data
  • return a small amount of data but generate a big SQL query
  • whatever else you can think of

Obviously we can all iterate on this and contribute queries as we go, and this will be an ongoing project

@jberryman
Copy link
Collaborator

Also mention in README, something like: "The graphql-engine log file is located in ./server/tests-py/remote_relationship_tests/test_output/hge.log and persists between runs"

@jberryman
Copy link
Collaborator

Another thing we should improve: abort if the stack build fails (else we will continue on with the wrong version)

`python3 test_with_sportsdb.py --help`
1) Option to skip stack build (--skip-stack-build)
2) GraphQL engine can be run using docker image (--hge-docker-image)
3) You can specify Postgres URLs as CSV instead of running them (--pg-urls)
4) You can choose the Postgres docker image (--pg-docker-image)

Apart from this test will now exit if stack build is required and stack build fails
tirumaraiselvan pushed a commit that referenced this pull request Apr 21, 2021
Co-authored-by: scriptonist <[email protected]>
GITHUB_PR_NUMBER: 6231
GITHUB_PR_URL: hasura#6231

Co-authored-by: scriptonist <[email protected]>
GitOrigin-RevId: c34d88b
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.

2 participants