open-telemetry / opentelemetry-python-contrib

OpenTelemetry instrumentation for Python modules
https://opentelemetry.io
Apache License 2.0
702 stars 587 forks source link

Add better messaging when supported library versions change between releases #1745

Open phillipuniverse opened 1 year ago

phillipuniverse commented 1 year ago

Is your feature request related to a problem?

Recently there have been a few version changes in libraries that have been instrumented. Specifically:

In my case, in order to maximize tracing adoption in my org I pull in all dependencies through the opentelemetry-contribut-instrumentations package, e.g.:

opentelemetry-contrib-instrumentations = { version = "0.38b0", optional = true }

This means that sometimes dependencies are pulled in that don't apply to some projects. For instance, we have some Django projects and some FastAPI projects. With this setup, on a FastAPI project both the Django and FastAPI instrumentation is pulled in, but it's a really nice feature that the FastAPI instrumentation is just skipped over in a Django project and vice-versa.

However, if FastAPI is in the venv then it's a big problem for us if FastAPI is not instrumented. This goes for all of our libraries that would normally have instrumentation, but may not because of version conflicts.

The only way you would know this is if you had DEBUG logging turned on in opentelemetry.instrumentation.auto_instrumentation:

[2023-04-06T16:22:34.060393-05:00] | DEBUG    | [service=tempus env=localdev version=no-rel-identifier-found trace_id=0 span_id=0] [opentelemetry.instrumentation.auto_instrumentation.sitecustomize] [sitecustomize.py:_load_instrumentors:76] - Skipping instrumentation httpx: DependencyConflict: requested: "httpx<=0.23.0,>=0.18.0" but found: "httpx 0.23.3"

Describe the solution you'd like

"Strict" or "Fail Fast" mode

I recognize that this problem is somewhat of a side-effect of being able to pull in all the instrumentations at once and have them fail flexibly. But what if there was like a "strict" mode or "fail loudly" mode or something, that differentiates between the case where the depednency is None vs a real version conflict.

For instance, in a FastAPI project I don't really care about this issue about Django, since that's not in my venv:

[2023-04-06T16:22:34.054898-05:00] | DEBUG    | [service=tempus env=localdev version=no-rel-identifier-found trace_id=0 span_id=0] [opentelemetry.instrumentation.auto_instrumentation.sitecustomize] [sitecustomize.py:_load_instrumentors:76] - Skipping instrumentation django: DependencyConflict: requested: "django>=1.10" but found: "None"

But if I've got httpx in my venv, I really care about this one:

[2023-04-06T16:22:34.060393-05:00] | DEBUG    | [service=tempus env=localdev version=no-rel-identifier-found trace_id=0 span_id=0] [opentelemetry.instrumentation.auto_instrumentation.sitecustomize] [sitecustomize.py:_load_instrumentors:76] - Skipping instrumentation httpx: DependencyConflict: requested: "httpx<=0.23.0,>=0.18.0" but found: "httpx 0.23.3"

Callout in release notes

Whenever these versions change, it seems like this is always tagged with "Skip Changelog". In my opinion, the opposite should be happening, it's worth a completely separate callout in the release that the required versions changed.

package_info vs instruments extra?

I think at one point this package._instruments was being used but it seems like that has been dropped in favor of an instruments extra. Sometimes these don't match. Is there a reason to have both?

Example being httpx:

Describe alternatives you've considered

Explicitly bringing in each instrumentation along with their instruments --extra. This would then solve the problem at dependency resolution time, however it does make it impossible for me to provide my own patches or potentially live more dangerously with unsupported versions.

Additional context

Hope that's enough information above!

phillipuniverse commented 1 year ago

Another idea on this, it could be nice to allow users to have a more formal way to accept dependency conflicts in their environment. I currently work around this with the following monkeypatch, e.g. to allow pscopg2 and httpx >= 0.18:

    orig_get_dependency_conflicts = (
        opentelemetry.instrumentation.dependencies.get_dependency_conflicts
    )

    def allowed_dependency_conflicts(
        deps: typing.Collection[str],
    ) -> DependencyConflict | None:
        if "psycopg2>=2.7.3.1" in deps:
            # Workaround for https://github.com/open-telemetry/opentelemetry-python-contrib/issues/610
            conflict = orig_get_dependency_conflicts(deps)
            if conflict and not conflict.found:
                return orig_get_dependency_conflicts(["psycopg2-binary>=2.7.3.1"])

        if "httpx<=0.23.0,>=0.18.0" in deps:
            # Workaround for https://github.com/open-telemetry/opentelemetry-python-contrib/issues/1463
            conflict = orig_get_dependency_conflicts(deps)
            if conflict:
                return orig_get_dependency_conflicts(["httpx>=0.18.0"])

        return allowed_dependency_conflicts(deps)

    opentelemetry.instrumentation.dependencies.get_dependency_conflicts = (
        shipwell_dependency_conflicts
    )