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.

Sign up for GitHub

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

Jump to bottom

bootstrap: no need to install tortoiseorm instrumentation if pydantic is found #2409

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

bootstrap: no need to install tortoiseorm instrumentation if pydantic is found #2409

wants to merge 4 commits into from

Conversation

xrmx
Copy link
Contributor

@xrmx xrmx commented Apr 11, 2024 •
edited
Loading

Description

Right now opentelemetry-bootstrap is installing tortoiseorm instrumentation if pydantic is found in the currently installed packages. In order to avoid that enhance bootstrap to recognize AND'ed requirements if instruments requirements are in a list.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • scripts/generate_instrumentation_bootstrap.py

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@xrmx xrmx requested a review from a team April 11, 2024 12:55
@xrmx xrmx force-pushed the no-need-tortoiseorm-instrumentation-for-pydantic branch from 89219d8 to 5561b62 Compare April 11, 2024 12:57
@xrmx
Copy link
Contributor Author

xrmx commented Apr 11, 2024

cc @tonybaloney

@xrmx xrmx marked this pull request as draft April 11, 2024 13:00
@xrmx xrmx force-pushed the no-need-tortoiseorm-instrumentation-for-pydantic branch 2 times, most recently from d0f7e7a to 8fa658e Compare April 12, 2024 09:56
@xrmx xrmx marked this pull request as ready for review April 12, 2024 10:04
@tammy-baylis-swi
Copy link
Contributor

Hi @xrmx. What use cases would this update fix? e.g. startup errors, duplicate spans, errored spans

ocelotl
ocelotl reviewed Apr 17, 2024
View reviewed changes
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Please give more context on how this PR is fixing an issue.

instrumentation/opentelemetry-instrumentation-tortoiseorm/pyproject.toml Show resolved Hide resolved
@xrmx
Copy link
Contributor Author

xrmx commented Apr 17, 2024 •
edited
Loading

Hi @xrmx. What use cases would this update fix? e.g. startup errors, duplicate spans, errored spans

bootstrap looks into this field for adding instrumentations based on the installed libraries. With that line tortoiseorm instrumentation is installed even in the case where just pydantic is installed.

@xrmx xrmx mentioned this pull request May 13, 2024
Open
@xrmx xrmx force-pushed the no-need-tortoiseorm-instrumentation-for-pydantic branch from 1800f0d to 69125d1 Compare May 27, 2024 16:02
@xrmx xrmx marked this pull request as draft May 27, 2024 16:06
@xrmx xrmx mentioned this pull request May 27, 2024
Merged
8 tasks
@xrmx
Copy link
Contributor Author

xrmx commented May 27, 2024 •
edited
Loading

Revamped the PR to teach bootstrap about AND'ed dependencies.
The idea here is to use lists instead of strings to express AND'ed dependencies (i.e. when the istrumentation requires more than one package to work) on top of the OR'ed dependencies supported right now. This is to fix tortoiseorm instrumentation that is installed by bootstrap even if the tortoiseorm library is not installed.
This PR is failling because the semantic for pyproject.toml optional dependencies table that we are using at the moment does not permit to use lists but just strings.
What do you think to stop using pyproject.toml optional dependencies table and use our own, e.g. [tool.opentelemetry-bootstrap]?
In order to have this work I had to move the table used in pyproject from [project.optional-dependencies] to [tool.opentelemetry-bootstrap] which I think is a good change by itself since what we store there are not the instrumentantion package optional dependencies.

@xrmx xrmx changed the title boostrap: no need to install tortoiseorm instrumentation if pydantic is found bootstrap: no need to install tortoiseorm instrumentation if pydantic is found May 27, 2024
@xrmx xrmx force-pushed the no-need-tortoiseorm-instrumentation-for-pydantic branch from 69125d1 to 778efea Compare May 28, 2024 08:35
@xrmx xrmx force-pushed the no-need-tortoiseorm-instrumentation-for-pydantic branch 4 times, most recently from 2ad194f to 7f41005 Compare June 6, 2024 08:41
xrmx added 4 commits June 6, 2024 15:00
@xrmx
bootstrap: handle packages that requires more than one library for in…
2919848 
…strumentations

Treat packages that express their project.optional-dependencies instruments
entry as a list to AND the requirements. This is to install the istrumentation
only if all the instruments packages are found.
More than one entry in project.optional-dependencies instruments are
still considered OR'ed as before.
@xrmx
script: handle lists on target instrumentations in generate_instrumen…
f4e4f54 
…tation_bootstrap.py
@xrmx
Move instruments group to tool.opentelemetry-bootstrap
da04459 
Remove the hack of using the project.optional-dependencies table for
expressing the libraries required for the instrumentation to work. This
frees us from the project.optional-dependencies specification that
permits to use only strings in dependencies groups.
@xrmx
tortoiseorm: use a list for instrumented packages
fc66e45 
Use a list to express that the tortoiseorm instrumentation targets
projects with both tortoiseorm AND pydantic installed.
So that tortoiseorm instrumentation is installed only if both packages
are found by opentelemetry-bootstrap.
@xrmx xrmx force-pushed the no-need-tortoiseorm-instrumentation-for-pydantic branch from 7f41005 to fc66e45 Compare June 6, 2024 13:01
@ocelotl ocelotl marked this pull request as ready for review June 6, 2024 16:06
xrmx
xrmx commented Jun 7, 2024
View reviewed changes
instrumentation/opentelemetry-instrumentation-aio-pika/pyproject.toml
@@ -29,7 +29,7 @@ dependencies = [
"wrapt >= 1.0.0, < 2.0.0",
]

[project.optional-dependencies]
[tool.opentelemetry-bootstrap]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this is breaking instrumentation.dependencies.get_dist_dependency_conflicts

Copy link
Contributor Author

@xrmx xrmx Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, it' breaking it. Given that AFAICS [tool.*] metadata is not available in installed package I guess we can use the _package module inside each instrumentation and maybe make [tool.opentelemetry-bootstrap] dynamic to avoid duplicating the data.

instrumentation.dependencies.get_dist_dependency_conflicts would then be updated to look at the _package module instead of the distribution requirements.

@xrmx xrmx mentioned this pull request Sep 5, 2024
Open
@xrmx
Copy link
Contributor Author

xrmx commented Nov 26, 2024

Another use case for this is #610

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocelotl ocelotl ocelotl left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@federicobond federicobond

@ocelotl ocelotl

@shalevr shalevr

@nikosokolik nikosokolik

@oxeye-nikolay oxeye-nikolay

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@xrmx @tammy-baylis-swi @ocelotl @federicobond @shalevr @nikosokolik @oxeye-nikolay