Re: [Tarantool-patches] [PATCH v1] bench-run: submit perf results into database

[Sergey Bronnikov <sergeyb@tarantool.org>]

Hello!

Oleg, thanks for the patch. See my 22 comments below.

On 13:18 Wed 22 Apr , Олег Пискунов wrote:
> 
> Resurrect bench.tarantool.org:
> - submit perf results into bench.tarantool.org database
>  
> Fix docker files for perf testing:
> - adding python "requests" module needed for publish script.
>  
> Closes #4865
>  
> Github: https://github.com/tarantool/bench-run/tree/opiskunov/gh-4865-submit-perf-results
> Issue: https://github.com/tarantool/tarantool/issues/4865
> ---
>  benchs/cbench/run.sh          | 38 ++++++++++++++++++------
>  benchs/linkbench/run.sh       | 13 +++++++--
>  benchs/nosqlbench/run.sh      | 22 ++++++++------
>  benchs/publication/publish.py | 67 +++++++++++++++++++++++++++++++++++++++++++
>  benchs/sysbench/run.sh        | 19 ++++++------
>  benchs/tpcc/run.sh            | 21 ++++++++------
>  benchs/ycsb/run.sh            | 16 +++++++++--
>  dockerfiles/ubuntu_benchs     |  4 +--
>  8 files changed, 158 insertions(+), 42 deletions(-)
>  create mode 100755 benchs/publication/publish.py
>  
> diff --git a/benchs/cbench/run.sh b/benchs/cbench/run.sh
> index 4d07972..c4867ad 100755
> --- a/benchs/cbench/run.sh
> +++ b/benchs/cbench/run.sh
> @@ -1,11 +1,11 @@
>  #!/usr/bin/env bash
>  
>  kill `pidof tarantool`
> +
>  set -e
>  set -o pipefail
>  
>  TAR_VER=$(tarantool -v | grep -e "Tarantool" |  grep -oP '\s\K\S*')
> -echo ${TAR_VER} | tee cbench_t_version.txt
>  
>  killall tarantool 2>/dev/null || true
>  rm -rf 5* 0*
> @@ -23,14 +23,34 @@ killall tarantool 2>/dev/null || true
>  rm -rf 5* 0*
>  sync

1. sync() can be asynchonous operation so we should wait it or somehow make
sure it has finished.

>  echo 3 > /proc/sys/vm/drop_caches
> -numactl --membind=1 --cpunodebind=1 --physcpubind=11 tarantool /opt/cbench/cbench_runner.lua vinyl write 500 2>&1 | tee cbench_output_memtx_write.txt
> +numactl --membind=1 --cpunodebind=1 --physcpubind=11 tarantool /opt/cbench/cbench_runner.lua vinyl write 500 2>&1 | tee cbench_output_vinyl_write.txt

2. What does '500' mean? Let's move magic numbers to constants.

> -grep "^?tab" cbench_output_memtx.txt | sed "s/.*name=//"| sed "s/&param=/:/"| sed "s/cb\./cb\.memtx\./"| tee -a cbench_result.txt
> -grep "^?tab" cbench_output_vinyl_fsync.txt | sed "s/.*name=//"| sed "s/&param=/:/"| sed "s/cb\./cb\.vinyl\.fsync\./"| tee -a cbench_result.txt
> -grep "^?tab" cbench_output_memtx_write.txt | sed "s/.*name=//"| sed "s/&param=/:/"| sed "s/cb\./cb\.vinyl\.write\./"| tee -a cbench_result.txt
> +grep "^?tab=cbench.tree" cbench_output_memtx.txt | sed "s/.*name=//"| sed "s/&param=/:/"| sed "s/cb\./cb\.memtx\./"| tee -a cbench-memtx-tree_result.txt
> +grep "^?tab=cbench.hash" cbench_output_memtx.txt | sed "s/.*name=//"| sed "s/&param=/:/"| sed "s/cb\./cb\.memtx\./"| tee -a cbench-memtx-hash_result.txt
> +grep "^?tab" cbench_output_vinyl_fsync.txt | sed "s/.*name=//"| sed "s/&param=/:/"| sed "s/cb\./cb\.vinyl\.fsync\./"| tee -a cbench-vinyl-fsync_result.txt
> +grep "^?tab" cbench_output_vinyl_write.txt | sed "s/.*name=//"| sed "s/&param=/:/"| sed "s/cb\./cb\.vinyl\.write\./"| tee -a cbench-vinyl-write_result.txt

3. As far as I understand cbench_runner.lua wrote two files
cbench_output_memtx_write.txt and cbench_output_vinyl_write.txt, you
pass filenames as arguments. On next step we should extract interested
numbers from these files. But cbench_output_memtx_write.txt is unused
here. And how other files were created: cbench_output_memtx.txt and
cbench_output_vinyl_fsync.txt?

4. Lines with grep/sed looks absolutely unreadable. Please split lines, like below:

grep "^?tab" cbench_output_memtx.txt |
		sed "s/.*name=//"| sed "s/&param=/:/"| sed "s/cb\./cb\.memtx\./" |
		tee -a cbench_result.txt
grep "^?tab" cbench_output_vinyl_fsync.txt |
		sed "s/.*name=//"| sed "s/&param=/:/"| sed "s/cb\./cb\.vinyl\.fsync\./"|
		tee -a cbench_result.txt

> -#mv tarantool-server.log cbench_tarantool_server.log
> -
> -echo "RESULTS:"
> -cat cbench_result.txt
> +echo ${TAR_VER} | tee cbench-memtx-tree_t_version.txt
> +echo ${TAR_VER} | tee cbench-memtx-hash_t_version.txt
> +echo ${TAR_VER} | tee cbench-vinyl-fsync_t_version.txt
> +echo ${TAR_VER} | tee cbench-vinyl-write_t_version.txt
> +echo ${TAR_VER} | tee cbench_t_version.txt
>
> +echo "Tarantool TAG:"
> +cat cbench_t_version.txt
> +echo "Overall results:"
> +echo "================"
> +echo "RESULTS (cbench-memtx-tree_result.txt):"
> +cat cbench-memtx-tree_result.txt
> +echo " "
> +echo "RESULTS (cbench-memtx-hash_result.txt):"
> +cat cbench-memtx-hash_result.txt
> +echo " "
> +echo "RESULTS (cbench-vinyl-fsync_result.txt):"
> +cat cbench-vinyl-fsync_result.txt
> +echo " "
> +echo "RESULTS (cbench-vinyl-write_result.txt):"
> +cat cbench-vinyl-write_result.txt
> +echo " "
> +echo "Publish data to bench database"
> +/opt/bench-run/benchs/publication/publish.py

5. Running of publish.py will broke if we will move repo to another
directory. Let's use relative path to publish.py.

> diff --git a/benchs/linkbench/run.sh b/benchs/linkbench/run.sh
> index 6738436..e7c77c3 100755
> --- a/benchs/linkbench/run.sh
> +++ b/benchs/linkbench/run.sh
> @@ -6,7 +6,6 @@ set -e

6. set -u

>  set -o pipefail
>  
>  TAR_VER=$(tarantool -v | grep -e "Tarantool" |  grep -oP '\s\K\S*')
> -echo ${TAR_VER} | tee linkbench_t_version.txt
>  
>  cd /opt/linkbench && mvn clean package -Dmaven.test.skip=true
>  cd src/tarantool && make
> @@ -34,4 +33,14 @@ sync

7. sync() can be asynchonous operation, we should wait it to make sure
it has finished.

>  echo 3 > /proc/sys/vm/drop_caches
>  $numaopts /opt/linkbench/bin/linkbench -c $cfgfile -r 2>&1 | tee linkbench_output.txt
>  
> -grep "REQUEST PHASE COMPLETED" linkbench_output.txt | sed "s/.*second = /linkbench:/" | tee -a linkbench_result.txt
> +grep "REQUEST PHASE COMPLETED" linkbench_output.txt | sed "s/.*second = /linkbench:/" | tee -a linkbench.ssd_result.txt
> +echo ${TAR_VER} | tee linkbench.ssd_t_version.txt
> +
> +echo "Tarantool TAG:"
> +cat linkbench.ssd_t_version.txt
> +echo "Overall results:"
> +echo "================"
> +cat linkbench.ssd_result.txt
> +echo " "
> +echo "Publish data to bench database"
> +/opt/bench-run/benchs/publication/publish.py

8. Running of publish.py will broke if we will move repo to another
directory. Let's use relative path to publish.py.

> diff --git a/benchs/nosqlbench/run.sh b/benchs/nosqlbench/run.sh
> index c9491f5..a68248e 100755
> --- a/benchs/nosqlbench/run.sh
> +++ b/benchs/nosqlbench/run.sh
> @@ -6,13 +6,11 @@ if [ "$type" == "" ]; then
>  fi
>  
>  TAR_VER=$(tarantool -v | grep -e "Tarantool" |  grep -oP '\s\K\S*')
> -echo ${TAR_VER} | tee nosqlbench_t_version.txt
>  
>  kill `pidof tarantool`
>  
> -#set -e
> -#
> -#set -o pipefail
> +set -e
> +set -o pipefail
>  
>  killall tarantool 2>/dev/null || true
>  rm -rf 0*.xlog 0*.snap

9. It is not clear why filemask includes zero at the beginning.

> @@ -42,8 +40,14 @@ numactl --membind=1 --cpunodebind=1 --physcpubind=6,7,8,9,10,11 /opt/nosqlbench/
>  #echo "Latest 1000 lines:"
>  #tail -1000 nosqlbench_output.txt

10. please remove commented lines or uncomment them
>
> -echo "Getting results"
> -grep "TOTAL RPS STATISTICS:" nosqlbench_output.txt -A6 | awk -F "|" 'NR > 4 {print $2,":", $4}' > nosqlbench_result.txt
> -
> -cat nosqlbench_result.txt
> -
> +grep "TOTAL RPS STATISTICS:" nosqlbench_output.txt -A6 | awk -F "|" 'NR > 4 {print $2,":", $4}' > noSQLbench.${type}_result.txt
> +echo ${TAR_VER} | tee noSQLbench.${type}_t_version.txt
> +
> +echo "Tarantool TAG:"
> +cat noSQLbench.${type}_t_version.txt
> +echo "Overall results:"
> +echo "================"
> +cat noSQLbench.${type}_result.txt
> +echo " "
> +echo "Publish data to bench database"
> +/opt/bench-run/benchs/publication/publish.py
> diff --git a/benchs/publication/publish.py b/benchs/publication/publish.py
> new file mode 100755
> index 0000000..d472fd7
> --- /dev/null
> +++ b/benchs/publication/publish.py

11. Running of publish.py will broke if we will move repo to another
directory. Let's use relative path to publish.py.

> @@ -0,0 +1,67 @@
> +#!/usr/bin/env python
> +import fnmatch
> +import os
> +from urllib import urlencode
> +import requests
> +
> +
> +def parse_bench(filename):
> +    fileHandle = open(filename)
> +    lastline = fileHandle.readlines()
> +    fileHandle.close()
> +    return lastline

12. you can implement the same body of function much simpler:

		with open(filename) as raw_data:
			return raw_data.readlines()
> +
> +
> +def get_version(filename):
> +    fileHandle = open(filename)
> +    lastline = fileHandle.readlines()[-1]
> +    fileHandle.close()
> +    return lastline.split()[0]

13. you can implement the same body of function much simpler:

		with open(filename) as raw_data:
			return raw_data.readlines()[-1][0]

> +def push_to_microb(server, token, name, value, version, tab):
> +    uri = 'http://%s/push?%s' % (server, urlencode(dict(
> +        key=token, name=name, param=value,
> +        v=version, unit='trps', tab=tab
> +    )))
> +
> +    r = requests.get(uri)
> +    if r.status_code == 200:
> +        print('Export complete')
> +    else:
> +        print('Export error http: %d' % r.status_code)
> +        print('Export error text: %d' % r.text)
> +
> +
> +def main():
> +    if "MICROB_WEB_TOKEN" in os.environ and "MICROB_WEB_HOST" in os.environ:
> +        bench = {}
> +        res = []
> +        current_data = {}
> +        version = ''
> +        for file in os.listdir('.'):
> +            if fnmatch.fnmatch(file, '*_result.txt'):
> +                values = parse_bench(file)
> +                benchmark = file.split('_')[0]
> +                version = get_version('{}_t_version.txt'.format(benchmark))
> +                for value in values:
> +                    test_name = value.split(':')[0]
> +                    test_res = float(value.split(':')[1])
> +                    res.append(test_res)
> +                    push_to_microb(
> +                        os.environ['MICROB_WEB_HOST'],
> +                        os.environ['MICROB_WEB_TOKEN'],
> +                        test_name,
> +                        test_res,
> +                        version,
> +                        benchmark,
> +                    )
> +        print ("VERSION - ", version)
> +    else:
> +        print("MICROB params not specified")
> +
> +    return 0
> +
> +
> +if __name__ == '__main__':
> +    main()
> diff --git a/benchs/sysbench/run.sh b/benchs/sysbench/run.sh
> index 6a69d14..ef0af43 100755
> --- a/benchs/sysbench/run.sh
> +++ b/benchs/sysbench/run.sh
> @@ -11,8 +11,6 @@ set -e
>  set -o pipefail
>  
>  TAR_VER=$(tarantool -v | grep -e "Tarantool" |  grep -oP '\s\K\S*')
> -echo ${TAR_VER} | tee sysbench_t_version.txt
> -set -e
>  
>  ARRAY_TESTS=(
>      "oltp_read_only"
> @@ -44,11 +42,10 @@ opts="--db-driver=${DBMS} --threads=${THREADS}"
>  
>  export LD_LIBRARY_PATH=/usr/local/lib
>  
> -rm -f sysbench_result.txt
> -set -o pipefail
> +rm -f Sysbench_result.txt
>  for test in "${ARRAY_TESTS[@]}"; do
>      res=0
> -    tlog=sysbench_${test}_result.txt
> +    tlog=sysbench_${test}_results.txt
>      rm -f $tlog
>      maxres=0
>      for run in `eval echo {1..$runs}` ; do
> @@ -69,7 +66,7 @@ for test in "${ARRAY_TESTS[@]}"; do
>          if [[ $tres -gt $maxres ]]; then maxres=$tres ; fi
>      done
>      res=$(($res/$runs))
> -    echo "${test}: $res" >>sysbench_result.txt
> +    echo "${test}: $res" >>Sysbench_result.txt
>      echo "Subtest '$test' results:"
>      echo "==============================="
> @@ -78,7 +75,13 @@ for test in "${ARRAY_TESTS[@]}"; do
>      printf "Diviations (AVG -> MAX): %.2f" `bc <<< "scale = 4; (1 - $res / $maxres) * 100"` ; echo %

14. Deviations

>  done
>
> +echo ${TAR_VER} | tee Sysbench_t_version.txt
> +
> +echo "Tarantool TAG:"
> +cat Sysbench_t_version.txt
>  echo "Overall results:"
>  echo "================"
> -cat sysbench_result.txt
> -
> +cat Sysbench_result.txt
> +echo " "
> +echo "Publish data to bench database"
> +/opt/bench-run/benchs/publication/publish.py

15. Running of publish.py will broke if we will move repo to another
directory. Let's use relative path to publish.py.

> diff --git a/benchs/tpcc/run.sh b/benchs/tpcc/run.sh
> index f1f60cc..76754e5 100755
> --- a/benchs/tpcc/run.sh
> +++ b/benchs/tpcc/run.sh
> @@ -1,15 +1,12 @@
>  #!/usr/bin/env bash
> +set -e

16. set -u

> +set -o pipefail
>  
>  TAR_VER=$(tarantool -v | grep -e "Tarantool" |  grep -oP '\s\K\S*')
> -echo ${TAR_VER} | tee tpcc_t_version.txt
> -
> -set -e
>  
>  if [ ! -n "${TIME}" ]; then TIME=1200; fi
>  if [ ! -n "${WARMUP_TIME}" ]; then WARMUP_TIME=10; fi
>  
> -set -o pipefail
> -
>  killall tarantool tpcc_load 2>/dev/null || true
>  rm -rf 0*.xlog 0*.snap
>  sync

17. the same comment as above regarding sync() and filemask for *.xlog and *.snap

> @@ -30,10 +27,16 @@ cd /opt/tpcc
>  numactl --membind=1 --cpunodebind=1 --physcpubind=6,7,8,9,10,11 \
>      /opt/tpcc/tpcc_start $tpcc_opts -r10 -l${TIME} -i${TIME} >tpcc_output.txt 2>/dev/null
>  
> -echo -n "tpcc:" | tee tpcc_result.txt
> -cat tpcc_output.txt | grep -e '<TpmC>' | grep -oP '\K[0-9.]*' | tee -a tpcc_result.txt
> -
> +echo -n "tpcc:" | tee tpc.c_result.txt
> +cat tpcc_output.txt | grep -e '<TpmC>' | grep -oP '\K[0-9.]*' | tee -a tpc.c_result.txt
>  cat tpcc_output.txt
> +echo ${TAR_VER} | tee tpc.c_t_version.txt
> +
> +echo "Tarantool TAG:"
> +cat tpc.c_t_version.txt
>  echo "Overall result:"
>  echo "==============="
> -cat tpcc_result.txt
> +cat tpc.c_result.txt
> +echo " "
> +echo "Publish data to bench database"
> +/opt/bench-run/benchs/publication/publish.py

18. Running of publish.py will broke if we will move repo to another
directory. Let's use relative path to publish.py.

> diff --git a/benchs/ycsb/run.sh b/benchs/ycsb/run.sh
> index 5ae1c75..d2cf8af 100755
> --- a/benchs/ycsb/run.sh
> +++ b/benchs/ycsb/run.sh
> @@ -17,7 +17,6 @@ set -e
>  set -o pipefail
>  
>  TAR_VER=$(tarantool -v | grep -e "Tarantool" |  grep -oP '\s\K\S*')
> -echo ${TAR_VER} | tee ycsb_t_version.txt
>  
>  ws=/opt/ycsb
>  cd $ws
> @@ -50,7 +49,18 @@ for l in a b c d e f ; do
>         echo 3 > /proc/sys/vm/drop_caches
>          numactl --membind=1 --cpunodebind=1 --physcpubind=6,7,8,9,10,11 bin/ycsb run tarantool -s -P workloads/workload${l} >${res}.log 2>&1 || cat ${res}.log

19. cbench script uses physcpubind equal to 1, why we set exact these
numbers and why physcpubind has not the same set of numbers?

20. What does "l" variable mean?

>          grep Thro ${res}.log | awk '{ print "Overall result: "$3 }' | tee ${res}.txt
> -        sed "s#Overall result#$l $r#g" ${res}.txt >>${plogs}/results.txt
> +        sed "s#Overall result#$l $r#g" ${res}.txt >>${plogs}/ycsb.${mode}_result.txt
>      done
>  done
> -cat ${plogs}/results.txt
> +
> +echo ${TAR_VER} | tee ycsb.${mode}_t_version.txt
> +cp -f ${plogs}/ycsb.${mode}_result.txt .
> +
> +echo "Tarantool TAG:"
> +cat ycsb.${mode}_t_version.txt
> +echo "Overall results:"
> +echo "================"
> +cat ycsb.${mode}_result.txt
> +echo " "
> +echo "Publish data to bench database"
> +/opt/bench-run/benchs/publication/publish.py

21. Running of publish.py will broke if we will move repo to another
directory. Let's use relative path to publish.py.

> diff --git a/dockerfiles/ubuntu_benchs b/dockerfiles/ubuntu_benchs
> index fd9acb4..2c2e717 100644
> --- a/dockerfiles/ubuntu_benchs
> +++ b/dockerfiles/ubuntu_benchs
> @@ -71,5 +71,5 @@ RUN luarocks install \
>      https://raw.githubusercontent.com/tarantool/gperftools/master/rockspecs/gperftools-scm-1.rockspec \
>      --local >build.log 2>&1 || ( cat build.log && false )

22. Why do we need gperftools here?

> -# benchmarks runners
> -RUN git clone https://github.com/tarantool/bench-run.git /opt/bench-run
> +# adding python dependency
> +RUN pip install requests
> --
> 1.8.3.1

-- 
sergeyb@