Python auto-instrumentation: handle musl based containers #3332

xrmx · 2024-10-07T13:55:27Z

Description:

Build and and inject musl based python auto-instrumentation if proper annotation is configured:

instrumentation.opentelemetry.io/otel-python-platform: "musl"

This takes a different approach that the stale PR at #2266:

does not change directory where python sdk is installed in the docker container for default distribution (glibc)

Link to tracking Issue(s):

Resolves: Python autoinstrumentation for musl libc based application containers #2264

Testing: unit tests and e2e are green. Tested locally on minikube that by deploying an operator from this branch, a custom image for the auto-instrumentation was able to get metrics (that depends on psutil that uses a binary extension) out of a python alpine container with the instrumentation.opentelemetry.io/otel-python-platform: "musl" annotation and a stacktrace without it.
Also tested that the glibc based is copied to the container if the instrumentation image does not have the musl one.

Documentation: Will add docs to opentelemetry.io

xrmx · 2024-10-08T08:23:23Z

tests/e2e-instrumentation/instrumentation-python-musl/00-install-collector.yaml

@@ -0,0 +1,22 @@
+apiVersion: opentelemetry.io/v1alpha1


Adding specific e2e tests is not needed because the e2e-test-app-python docker image is already based on alpine right? looks like tests are failing on main

looks like tests are failing on main

Can you elaborate this a bit more?

Adding specific e2e tests is not needed because the e2e-test-app-python docker image is already based on alpine right?

Maybe this was something where I failed. Our idea is, at some point, add verifications to know if the libraries were injected properly and verify they are emitting data.

At the moment the e2e-test-app-python is based on alpine but the python instrumentation image is glibc based. This is a problem because binary extensions are not portable between different C libraries (among other incompatibilities). So this PR builds them and copies one for musl or glibc depending on the configuration.

An example of failure in CI is this:
https://github.com/open-telemetry/opentelemetry-operator/actions/runs/11237912151/job/31241432422?pr=3330#step:8:1330

Where I guess the metrics thread kicks in and the system metrics package fails to load psutil binary module because it has been built on glibc and not musl.

BTW the other thing that should be kept in sync is the Python version of the two images because the ABI changes between python versions.

At the moment the e2e-test-app-python is based on alpine but the python instrumentation image is glibc based. This is a problem because binary extensions are not portable between different C libraries (among other incompatibilities). So this PR builds them and copies one for musl or glibc depending on the configuration.

Didn't notice this when I added the images. As mentioned in the previous comment, the idea is to add real E2E checking if the instrumentation is generating real data. Since we are not checking this, issues like the one you saw are happening. This is something we need to fix with that image.

You can reuse that image (since it is musl based) for your E2E test. We need to add a new one for glibc.

You can reuse that image (since it is musl based) for your E2E test. We need to add a new one for glibc.

I'm already using that image in the musl e2e 👍

iblancasa

Missing changelog.

iblancasa · 2024-10-10T10:33:20Z

tests/e2e-instrumentation/instrumentation-python-musl/00-install-collector.yaml

@@ -0,0 +1,22 @@
+apiVersion: opentelemetry.io/v1alpha1


looks like tests are failing on main

Can you elaborate this a bit more?

Adding specific e2e tests is not needed because the e2e-test-app-python docker image is already based on alpine right?

Maybe this was something where I failed. Our idea is, at some point, add verifications to know if the libraries were injected properly and verify they are emitting data.

swiatekm

Could you have a look at the e2e test failures?

pkg/instrumentation/annotation.go

xrmx · 2024-10-15T10:24:34Z

AFAICS e2e tests are failing because the python autoinstrumentation docker image does not have the musl based installation, is it the case because the image used in tests is not built from git?

swiatekm · 2024-10-15T10:51:42Z

AFAICS e2e tests are failing because the python autoinstrumentation docker image does not have the musl based installation, is it the case because the image used in tests is not built from git?

Autoinstrumentation tests use the default image, we don't build all the autoinstrumentation images from source for E2E tests, though we probably should.

What I would suggest here:

Verify locally that your current tests pass with the new image
Modify tests in this PR to only check if the container manifest is correct (and not that it's running)
Merge this PR
Operator release happens
Open a new PR, changing the tests back to their current state

I know it's a hassle, but it's simpler than all the alternatives.

xrmx · 2024-10-15T19:40:18Z

AFAICS e2e tests are failing because the python autoinstrumentation docker image does not have the musl based installation, is it the case because the image used in tests is not built from git?

Autoinstrumentation tests use the default image, we don't build all the autoinstrumentation images from source for E2E tests, though we probably should.

What I would suggest here:
1. Verify locally that your current tests pass with the new image

2. Modify tests in this PR to only check if the container manifest is correct (and not that it's running)

3. Merge this PR

4. Operator release happens

5. Open a new PR, changing the tests back to their current state
I know it's a hassle, but it's simpler than all the alternatives.

Other than fixing the tests though this is breaking backward compatibility with older images and maybe we can avoid that?

xrmx · 2024-10-18T09:24:32Z

Ok finally tested this manually by deploying an operator from this branch, a custom image for the auto-instrumentation and was able to get metrics (that depends on psutil that uses a binary extension) out of a python alpine container with the instrumentation.opentelemetry.io/otel-python-platform: "musl" annotation and a stacktrace without it.

Also tested that the glibc based is copied to the container if the instrumentation image does not have the musl one.

autoinstrumentation/python/Dockerfile

pmcollins

I'm not an approver here but this looks great @xrmx. Do we have a place where we can document any of this for users of this functionality?

xrmx · 2024-10-23T07:08:44Z

I'm not an approver here but this looks great @xrmx. Do we have a place where we can document any of this for users of this functionality?

Thanks for reviewing, will update https://opentelemetry.io/docs/kubernetes/operator/automatic/ and https://opentelemetry.io/docs/zero-code/python/operator/

pkg/instrumentation/python.go

iblancasa

Missing changelog.

xrmx · 2024-10-29T10:35:12Z

Missing changelog.

It's the first hunk in the diff

tests/e2e-instrumentation/instrumentation-python-musl/01-assert.yaml

swiatekm

The changes look good to me, thanks for being patient with our feedback on this PR! One small thing I think is still missing is documenting this new annotation in the README here: https://github.com/open-telemetry/opentelemetry-operator?tab=readme-ov-file#opentelemetry-auto-instrumentation-injection.

Build and and inject musl based python auto-instrumentation if proper annotation is configured: instrumentation.opentelemetry.io/otel-python-platform: "musl" Refs open-telemetry#2264

xrmx · 2024-11-04T13:03:57Z

The changes look good to me, thanks for being patient with our feedback on this PR! One small thing I think is still missing is documenting this new annotation in the README here: https://github.com/open-telemetry/opentelemetry-operator?tab=readme-ov-file#opentelemetry-auto-instrumentation-injection.

Updated README and rebased, thanks!

I think you have been more patient with me than the other way around 😅

xrmx requested a review from a team as a code owner October 7, 2024 13:55

xrmx force-pushed the python-multi-libc-distribution-2 branch 2 times, most recently from 6e778ef to c9d17f7 Compare October 7, 2024 14:03

xrmx commented Oct 8, 2024

View reviewed changes

iblancasa reviewed Oct 10, 2024

View reviewed changes

xrmx force-pushed the python-multi-libc-distribution-2 branch from e8640b7 to b6d469b Compare October 10, 2024 13:17

swiatekm reviewed Oct 14, 2024

View reviewed changes

pkg/instrumentation/annotation.go Outdated Show resolved Hide resolved

xrmx force-pushed the python-multi-libc-distribution-2 branch from b6d469b to c6ce177 Compare October 15, 2024 09:59

xrmx marked this pull request as draft October 17, 2024 12:21

xrmx force-pushed the python-multi-libc-distribution-2 branch from b60ddfe to 42b4ad4 Compare October 18, 2024 07:30

xrmx marked this pull request as ready for review October 18, 2024 09:19

xrmx requested review from swiatekm and iblancasa October 18, 2024 09:24

xrmx commented Oct 18, 2024

View reviewed changes

autoinstrumentation/python/Dockerfile Outdated Show resolved Hide resolved

pmcollins approved these changes Oct 22, 2024

View reviewed changes

swiatekm reviewed Oct 23, 2024

View reviewed changes

pkg/instrumentation/python.go Outdated Show resolved Hide resolved

xrmx mentioned this pull request Oct 23, 2024

autoinstrumentation: install musl based autoinstrumentation in Python Docker image #3384

Merged

xrmx marked this pull request as draft October 23, 2024 12:45

xrmx force-pushed the python-multi-libc-distribution-2 branch 3 times, most recently from 8521ae3 to bdf97ef Compare October 29, 2024 08:11

xrmx marked this pull request as ready for review October 29, 2024 08:12

xrmx requested a review from swiatekm October 29, 2024 08:23

iblancasa approved these changes Oct 29, 2024

View reviewed changes

swiatekm reviewed Oct 31, 2024

View reviewed changes

tests/e2e-instrumentation/instrumentation-python-musl/01-assert.yaml Show resolved Hide resolved

swiatekm reviewed Nov 4, 2024

View reviewed changes

xrmx added 5 commits November 4, 2024 13:58

Python auto-instrumentation: handle musl based containers

edac089

Build and and inject musl based python auto-instrumentation if proper annotation is configured: instrumentation.opentelemetry.io/otel-python-platform: "musl" Refs open-telemetry#2264

Add changelog

a182888

fix indentation in e2e yaml

aceb408

Assert specific command in musl e2e instrumentation test

d0acfb8

Update README

727b82b

xrmx force-pushed the python-multi-libc-distribution-2 branch from cf7c491 to 727b82b Compare November 4, 2024 13:02

swiatekm approved these changes Nov 4, 2024

View reviewed changes

swiatekm enabled auto-merge (squash) November 4, 2024 13:06

swiatekm merged commit 2389f94 into open-telemetry:main Nov 4, 2024
36 checks passed

alexkruc mentioned this pull request Dec 5, 2024

Merge from Upstream (#1) #3514

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python auto-instrumentation: handle musl based containers #3332

Python auto-instrumentation: handle musl based containers #3332

xrmx commented Oct 7, 2024 •

edited

Loading

xrmx Oct 8, 2024 •

edited

Loading

iblancasa Oct 10, 2024

xrmx Oct 10, 2024 •

edited

Loading

xrmx Oct 10, 2024 •

edited

Loading

iblancasa Oct 10, 2024

xrmx Oct 14, 2024

iblancasa left a comment

iblancasa Oct 10, 2024

swiatekm left a comment

xrmx commented Oct 15, 2024

swiatekm commented Oct 15, 2024

xrmx commented Oct 15, 2024

xrmx commented Oct 18, 2024 •

edited

Loading

pmcollins left a comment

xrmx commented Oct 23, 2024

iblancasa left a comment

xrmx commented Oct 29, 2024

swiatekm left a comment

xrmx commented Nov 4, 2024

Python auto-instrumentation: handle musl based containers #3332

Python auto-instrumentation: handle musl based containers #3332

Conversation

xrmx commented Oct 7, 2024 • edited Loading

xrmx Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

iblancasa Oct 10, 2024

Choose a reason for hiding this comment

xrmx Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

xrmx Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

iblancasa Oct 10, 2024

Choose a reason for hiding this comment

xrmx Oct 14, 2024

Choose a reason for hiding this comment

iblancasa left a comment

Choose a reason for hiding this comment

iblancasa Oct 10, 2024

Choose a reason for hiding this comment

swiatekm left a comment

Choose a reason for hiding this comment

xrmx commented Oct 15, 2024

swiatekm commented Oct 15, 2024

xrmx commented Oct 15, 2024

xrmx commented Oct 18, 2024 • edited Loading

pmcollins left a comment

Choose a reason for hiding this comment

xrmx commented Oct 23, 2024

iblancasa left a comment

Choose a reason for hiding this comment

xrmx commented Oct 29, 2024

swiatekm left a comment

Choose a reason for hiding this comment

xrmx commented Nov 4, 2024

xrmx commented Oct 7, 2024 •

edited

Loading

xrmx Oct 8, 2024 •

edited

Loading

xrmx Oct 10, 2024 •

edited

Loading

xrmx Oct 10, 2024 •

edited

Loading

xrmx commented Oct 18, 2024 •

edited

Loading