opentelemetry-python-contrib icon indicating copy to clipboard operation
opentelemetry-python-contrib copied to clipboard

Consider altering "instruments" set to avoid breaking changes

Open jeremydvoss opened this issue 1 year ago • 4 comments

What problem do you want to solve?

A recent PR (fixed in https://github.com/open-telemetry/opentelemetry-python-contrib/pull/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:

  1. Clearly document that "instruments" is an "and list" that expects all listed dependencies to be installed
  2. 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"
  3. Add a new list like "or-instruments" and check both. The relationship would need to be speced out more
  4. Allow instrumentations to choose between the existing get_dist_dependency_conflicts or a new one that treats instruments as an "or list".
  5. Explore whether get_distribution might work with more complex "instruments" dependencies like:
instruments = [
    "fastapi ~= 0.58 OR fastapi-slim ~= x.y,
]
  1. 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

jeremydvoss avatar Sep 04 '24 22:09 jeremydvoss