You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
A recent PR (fixed in #2783) broke the fastapi instrumentation. This was because "instruments" was treated as a set of dependencies any of which would trigger instrumentation. However, get_dist_dependency_conflicts instead treats any missing dependency as a "conflict". Therefore, the FastAPI instrumentation would only be triggered if the user has fastapi and fastapi-slim installed. The PR seems to have though that the instrumentation should be triggered if the user fastapi or fastapi-slim.
I feel this issue is bound to come up again. There may be future scenarios where we want to include an or in "instruments"
Describe the solution you'd like
No matter what, we need better testing.
Additionally, some possible solutions include:
Clearly document that "instruments" is an "and list" that expects all listed dependencies to be installed
Make "instruments" is an "or list" instead by changing get_dist_dependency_conflicts. (I don't see the major benefit of keeping it as an "and list"
Add a new list like "or-instruments" and check both. The relationship would need to be speced out more
Allow instrumentations to choose between the existing get_dist_dependency_conflicts or a new one that treats instruments as an "or list".
Explore whether get_distribution might work with more complex "instruments" dependencies like:
Enforce that every instrumentation should be for a single package. This would mean that supporting something like fastapi-slim would require a new instrumentation package with a different insturments list
Describe alternatives you've considered
No response
Additional Context
No response
Would you like to implement a fix?
None
The text was updated successfully, but these errors were encountered:
So, probably a dumb question but why have more than one package in instruments? Should we have a 1:1 relationship between instrumentations and packages?
So, probably a dumb question but why have more than one package in instruments?
Currently that's the case when an instrumentation requires two packages installed, e.g. tortoiseorm or when the same instrumentation can work with two different packages, e.g. kafka-python
Should we have a 1:1 relationship between instrumentations and packages?
The kafka-python / kafka-python-ng usecase is legit, in my opinion it really does not make sense to duplicate a whole instrumentation just for some metadata
What problem do you want to solve?
A recent PR (fixed in #2783) broke the fastapi instrumentation. This was because "instruments" was treated as a set of dependencies any of which would trigger instrumentation. However, get_dist_dependency_conflicts instead treats any missing dependency as a "conflict". Therefore, the FastAPI instrumentation would only be triggered if the user has fastapi and fastapi-slim installed. The PR seems to have though that the instrumentation should be triggered if the user fastapi or fastapi-slim.
I feel this issue is bound to come up again. There may be future scenarios where we want to include an or in "instruments"
Describe the solution you'd like
No matter what, we need better testing.
Additionally, some possible solutions include:
Describe alternatives you've considered
No response
Additional Context
No response
Would you like to implement a fix?
None
The text was updated successfully, but these errors were encountered: