support opentelemetry python#42
Conversation
DaGeRe
left a comment
There was a problem hiding this comment.
Could you please address the mentioned issues?
| executeAllLoops | ||
|
|
||
| deactivate | ||
| rm "$CONFIG_FILE" 2>/dev/null |
There was a problem hiding this comment.
Put this in .gitignore, do not delete the config file (except there is a specific reason)
| export RESULTS_DIR="$BASE_DIR/results-OpenTelemetry-python" | ||
| export RAWFN="$RESULTS_DIR/raw" | ||
|
|
||
| KIEKER_REPO_URL="https://github.com/kieker-monitoring/kieker-lang-pack-python.git" |
There was a problem hiding this comment.
This shouldn't be necessary for Otel Python
| KIEKER_REPO_URL="https://github.com/kieker-monitoring/kieker-lang-pack-python.git" | ||
| KIEKER_DIR="$BASE_DIR/kieker-lang-pack-python" | ||
|
|
||
| export PYTHON_SCRIPT="$MAIN_DIR/tools/pybenchmark/benchmark.py" |
There was a problem hiding this comment.
Could you name this MOOBENCH_BIN_PY (like in config.rc of Kieker-python)?
|
|
||
| export VENV_DIR="$BASE_DIR/venv" | ||
| export REQUIREMENTS_FILE="$BASE_DIR/requirements.txt" | ||
| export CONFIG_TEMPLATE="$BASE_DIR/config.ini.template" |
There was a problem hiding this comment.
Please try whether this can be removed as well.
| KIEKER_DIR="$BASE_DIR/kieker-lang-pack-python" | ||
|
|
||
| export PYTHON_SCRIPT="$MAIN_DIR/tools/pybenchmark/benchmark.py" | ||
| export ZIPKIN_BIN="$BASE_DIR/zipkin.jar" |
There was a problem hiding this comment.
This line can probably just be removed if you use startZipkin and stopBackgroundProcess.
| @@ -0,0 +1,90 @@ | |||
| MOOBENCH_CONFIGURATIONS="0 1 2" | |||
| TITLE[0]="No Instrumentation" | |||
There was a problem hiding this comment.
Could you move these into label.sh?
.gitignore
Outdated
| frameworks/pinpoint-java/pinpoint/pinpoint.tar.gz | ||
| frameworks/pinpoint-java/scripts/*.json | ||
| zipkin.jar | ||
| zipkin.jar |
There was a problem hiding this comment.
I have removed the duplicate.
| @@ -0,0 +1,85 @@ | |||
| export RECURSION_DEPTH="${RECURSION_DEPTH:-10}" | |||
There was a problem hiding this comment.
Don't duplicate this - it should be defined from benchmark.sh already
There was a problem hiding this comment.
I have removed the duplicate.
|
Thanks for the update, I've made two comments. Could you check them, remove the two duplicates, and respond to the existing comment? (If its not possible to address the comment, just reply there and describe why). |
|
Hi, thanks for the efforts! adding a new framework entry is not easy, because MooBench is an established framework that runs several tracing agents. Also, Kieker-python is still under developement. So I'm encouraged to see the work. From a quick glance, I want to make some points.
if [ -f "${MAIN_DIR}/init.sh" ] ; then
source "${MAIN_DIR}/init.sh"
else
echo "Missing library: ${MAIN_DIR}/init.sh"
exit 1
fiThen, you don't need: # config.rc
export RESULTS_DIR="$BASE_DIR/results-OpenTelemetry-python"
export RAWFN="$RESULTS_DIR/raw"# benchmark.sh
export NUM_OF_LOOPS=${NUM_OF_LOOPS:-10}
export TOTAL_NUM_OF_CALLS=${TOTAL_NUM_OF_CALLS:-2000000}
export RECURSION_DEPTH=${RECURSION_DEPTH:-10}
export METHOD_TIME=${METHOD_TIME:-0}
export SLEEP_TIME=${SLEEP_TIME:-15} |
|
Hello, thank you for the suggestions. I have refactored benchmark.sh to source init.sh as requested. I also removed export statements from config.rc and functions.sh and added the Windows/Cygwin path detection logic. Finally, I verified the benchmark still runs successfully with accurate measurements after the changes. |
d55accb to
5539f6b
Compare
|
Thanks for the PR. There are some small things to improve (especially formatting with shfmt), but since it's functional, I'll merge for now. |
with this pull request opentelemetry python instrumentation measurement becomes possible