Skip to content
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

Install all transitive dependencies for Python instrumentation #1483

Conversation

rapphil
Copy link
Contributor

@rapphil rapphil commented Aug 21, 2024

Fixes: #1482

This change will install all Python transitive dependencies of the instrumentation supported by the lambda layer. This has to be done to reduce the friction for users, so that they are not required to install extra dependencies for their instrumentation to work.

There are some exceptions that will be handled separately because they are depending on a pinned version of opentelemetry-propagator-aws-xray. We wont need to keep those exceptions in the next release because it is already fixed open-telemetry/opentelemetry-python-contrib#2773

@rapphil rapphil requested a review from a team August 21, 2024 20:27
opentelemetry-instrumentation-starlette==0.47b0
opentelemetry-instrumentation-tornado==0.47b0
opentelemetry-instrumentation-wsgi==0.47b0
# TODO: move these dependencies to requirements.txt when they stopped relying on a pinned version of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is already merged we can make a new Python release and this won't be necessary.

Copy link
Contributor Author

@rapphil rapphil Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this will work for v.0.48b0 (which is not released yet.). being merged does not mean that it is released.

@rapphil rapphil requested a review from ocelotl August 21, 2024 22:25
opentelemetry-propagator-aws-xray==1.0.2

# Instrumentation dependencies
opentelemetry-instrumentation-aiohttp-client==0.47b0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much does moving all this to the lambda layer increase the layer size? Is that enough to be concerned about for adding to the cold start time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simulated locally to check what is the delta:

without installing all dependencies

python3 -m pip install -r otel_sdk/requirements.txt -t ./deps && \
  python3 -m pip install -r otel_sdk/nodeps-requirements.txt -t ./deps --no-deps
du -h --max-depth 0 deps
25M     deps

installing all transitive dependencies

python3 -m pip install -r otel_sdk/requirements.txt -t ./deps && \
  python3 -m pip install -r otel_sdk/nodeps-requirements.txt -t ./deps --no-deps
du -h --max-depth 0 deps
27M     deps

The impact on the cold startup will be negligible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the last version was 7.4 MB. We can see where things end up with the next release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the data presented before regarding size was just for the dependencies and it was not zipped. Here is the resulting layer for reference:

8.3M Aug 22 15:51 build/opentelemetry-python-layer.zip

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So an added 1mb.... that's not insignificant. 🤷

Copy link
Member

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the no-deps idea was to allow users to only add dependency for the specific instrumentation they want, but bundling it all makes it easier to keep versions in sync.

@rapphil rapphil merged commit b284186 into open-telemetry:main Aug 22, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python "packaging" is required
3 participants