[chore] Build docker image for PHP auto-intrumentation #3409
Conversation
| password: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Prepare files for docker image | ||
| run: ./autoinstrumentation/php/prepare_files_for_docker_image.sh --ext-ver ${{ env.VERSION }} --dest-dir ${PWD}/autoinstrumentation/php/files_for_docker_image |
There was a problem hiding this comment.
How about running this inside the Dockerfile? As part of a multi-stage build
There was a problem hiding this comment.
Thank you - good idea. I'll try - I wonder if it will work considering that the script runs other docker containers...
There was a problem hiding this comment.
I tried running the script in Dockerfile during build but in order for it to work /var/run/docker.sock has to be mounted from the host (because the script spawns docker containers to build various files). Unfortunately it seems it is not possible to mount /var/run/docker.sock from the host during the build phase of docker image.
There was a problem hiding this comment.
if it runs other containers, I think it doesnt make much sense to mount the docker sock. It simply makes it harder to execute it on machines with only e.g. podman available.
There was a problem hiding this comment.
I'd like to second the request to run all of this as a multi-stage docker build instead. That will make it much easier to maintain. I see that the script requires running docker images - in my view, you should instead rework it so the Dockerfile itself accepts the PHP version and libc flavor as arguments. If you can't fit everything in a single Dockerfile, multiple different ones are also fine.
There was a problem hiding this comment.
Thank you for clarification. I understand that multiple images built by the workflow using QEMU each image for the corresponding CPU architecture, the part about which I am not clear is how the corresponding image is selected at the runtime? Namely how pkg/instrumentation/python.go knows which image to copy files from? Will the correct image with auto-instrumentation files be selected based on CPU architecture used by docker image with the instrumented application? Do I understand correctly that that determination will occur after pkg/instrumentation/python.go execution?
There was a problem hiding this comment.
And on a separate note, could you cleanly rebase your changes on
main? Right now it looks like you have a very messy merge in there.
Yes, I did rebase your changes on main - should I have done a merge instead of rebase? Is there a way to fix the current messy state?
There was a problem hiding this comment.
That said, looking at your build script more, I can see why it wouldn't be that easy to switch to that method
Won't switching to multi-stage image approach (either by generating Dockerfile or writing it manually) handle CPU architecture automatically?
There was a problem hiding this comment.
Thank you for clarification. I understand that multiple images built by the workflow using QEMU each image for the corresponding CPU architecture, the part about which I am not clear is how the corresponding image is selected at the runtime? Namely how pkg/instrumentation/python.go knows which image to copy files from? Will the correct image with auto-instrumentation files be selected based on CPU architecture used by docker image with the instrumented application? Do I understand correctly that that determination will occur after pkg/instrumentation/python.go execution?
The operator doesn't know the CPU architecture of the image. It could find out, but it doesn't care what it is. The container runtime on the K8s Node (containerd in most cases) will simply download the right image for the Node's CPU architecture.
Won't switching to multi-stage image approach (either by generating Dockerfile or writing it manually) handle CPU architecture automatically?
It will, but a Dockerfile with 6+ build stages, one for each combination of libc+php, is going to be messy and repetitive. I'm wondering if there's an elegant way of handing this.
There was a problem hiding this comment.
I agree that neither of the discussed proposals is perfect but this docker image is just a bag of files and it's not used directly by the end user.
Maybe for now we can implement one of the proposals that is good enough, we will get the feature out, receive feedback and then if/when necessary we can improve upon it?
SergeyKleyman
changed the title
|
I marked autoinstrumentation/php/prepare_files_for_docker_image.sh as executable and added "[chore]" to the PR title to skip changelog check. Please take a look. |
|
@SergeyKleyman can you clean this branch up so Github doesn't show all the changes from main? |
Sure thing - I am not an Git expert though so maybe you can help me out with exact git commands to achieve that? I think I caused the current situation because I rebased my branch on main and then I did pull-then-push while maybe the right way would have been to force-push without pull ? I just thought that force-pushing to published branch is kind of "bad" thing. So maybe you can help me with the following two questions:
Thank you in advance. |
It seems merging main to my branch fixed the issue so I assume I should have just merged main on the first attempt instead of rebasing on main. |
Since PHP 8.0 support was removed from OpenTelemetry for PHP SDK (from version 1.1)
|
@swiatekm Could you please take a look if there is anything else still needs to be done? |
There was a problem hiding this comment.
I think this LGTM. the bash code was actually incredibly readable so good on you :D
One minor thing, let's include php in the versions.txt file which rn just won't be read by anything.
Done. Please take a look. |
|
Could you please let me know what is the next step to get this PR merged? |
|
@pavolloffay could you also have a look? @SergeyKleyman no need to do anything else on your end. Supporting autoinstrumentation in a new language is simply a big decision, and we want to have wide consensus before merging it. |
Description: Part 1 of #3331 that is adding support for auto-instrumentation of PHP application in the same way as it's already implemented for other runtimes such as Java, .NET, etc.
As requested this PR handles the part which is publishing the image.