-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
adds USDT trace points for supported distros #366
Conversation
Just to put some stats down so we have a couple comparisons:
Overall, it might be worth the extra build time and small bump in size, tracing is nice to have. Is there a comparison of python performance before and after this flag? On the other branch, I'm not sure vendoring in the Debian scripts for Alpine is the right way; that could dramatically increase the maintenance in this repo and distracts from its simplicity. Maybe we just skip Alpine for now and work to get the package into Alpine proper? |
I can put together a benchmark. Will have that in the next day or two.
Seems like a reasonable approach. |
2.7/jessie/Dockerfile
Outdated
@@ -18,6 +18,7 @@ ENV PYTHONIOENCODING UTF-8 | |||
# extra dependencies (over what buildpack-deps already includes) | |||
RUN apt-get update && apt-get install -y --no-install-recommends \ | |||
tk-dev \ | |||
systemtap-sdt-dev \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this provide any user-facing benefit when we keep it installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's a miss. Will fix.
Seems generally sane, although I'm definitely curious to see the results of that benchmark. 👍 This is definitely supported for Python 2.x and 3.x? (https://docs.python.org/2/howto/instrumentation.html is a 404, so I want to make sure Python 2 isn't just quietly ignoring the More concretely: https://travis-ci.org/docker-library/python/jobs/481912534#L1050
(So I think this should definitely be removed from all the |
Sorry about the delay in replying. I looked into the Python2 thing and it looks like it's not supported in upstream for Linux, only as patches for other Unixes. So we can remove that. I'm going to admit I'm not sure what the I'm currently running Python's own benchmarking suite against both Not sure how long this benchmark takes to run but I'm just running it on a machine overnight and I'll post again here tomorrow with results. |
The python benchmark suite I ran exercises 60 real-world applications. It takes a bit over 20 min to run on my lab machine, so I ran this a few times to verify the results I got here. Bottom line: although many of the benchmarks don't show a significant difference between Needless to say I'm surprised and disappointed by this. Keep in mind this benchmark was run without an active trace, so I would not have expected the no-op function wrapping to create such a high level of overhead. I'm definitely glad I did the benchmarks, because accepting this PR would result in a significant community-wide hit. I'm going to withdraw this PR. There's another approach worth exploring for eBPF tracing of the interpreter without compiling-in usdt, so I may try to tackle publishing a reproducible method for that for folks to use. But that's not specific to Docker. Sorry for wasting your time @yosifkit & @tianon! 😊 But at least we have this data now for the next time someone suggests this. Benchmark results
Test runner: #!/bin/bash
set -e
cat <<EOF > Dockerfile
FROM python:3.7-stretch
RUN python3 -m pip install performance
EOF
cat <<EOF > Dockerfile-with-dtrace
FROM 0x74696d/python:3.7-stretch
RUN python3 -m pip install performance
EOF
docker build -t="python-bench-no-dtrace" .
docker build -t="python-bench-with-dtrace" -f Dockerfile-with-dtrace .
echo 'Running benchmark for Python3 baseline'
docker run -d \
--name bench1 \
-w /tmp \
-v $(pwd):/tmp \
python-bench-no-dtrace \
pyperformance run --python=python3 -o python-no-dtrace.json
time docker wait bench1
echo 'Running benchmark for Python3 with dtrace, no traces enabled'
docker run -d \
--name bench2 \
-w /tmp \
-v $(pwd):/tmp \
python-bench-with-dtrace \
pyperformance run --python=python3 -o python-dtrace-not-tracing.json
time docker wait bench2
pyperformance compare python-no-dtrace.json python-dtrace-not-tracing.json > results.txt |
Indeed, thanks for your time (and absolute thoroughness) on this, @tgross! ❤️ |
@tgross what is this other approach? Thanks! |
Instrumenting the function entry/exit the tracepoints wrap with a uprobe. I didn't find this was feasible though; it's not portable and a lot of trouble in any event. If you want instrumentation of the Python runtime without usdt you might want to consider a stochastic profiler such as |
Python supports user-level statically defined tracing (USDT) for platforms that support it. This has historically been not compiled-in by default in Linux distributions because only hosts with
systemtap
can take advantage of it. Newer Linux kernels (including all kernels supported by Docker) have eBPF, and this allows for all users to take advantage of tools like pythonflow, pythongc, etc. Including this option would have the happy side-effect of allowing for the USDT probes to be used for DTrace on platforms where Docker containers can be run on Illumos (ex. Joyent's Triton).The upstream implementation is a little weird (ref https://docs.python.org/3/howto/instrumentation.html). The
--with-dtrace
flag expects thedtrace
binary to exist so that appropriate header files can be written. Thesystemtap-sdt-dev
package for Debian provides a mock "dtrace
binary" (actually a Python script) which can generate the header files.This package or one like it is not available for Alpine. I have another branch with an experiment to vendor the header files and the mock
dtrace
binary from the Debiansystemtap-sdt-dev
package. This works inasmuch as the header gets created and I've verified the USDT probes work, but it's a totally gross approach and perhaps dubious from a licensing standpoint so I didn't include it in this PR. But I'd love to have a discussion about what the preferred approach would be.