open-telemetry / opentelemetry-python-contrib

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

Monkey patching vs upstream in-library instrumentation? #844

Open hterik opened 2 years ago

hterik commented 2 years ago

I got curious about the recommended way to add instrumentation to a new third party library.

Looking at many of the instrumentors here, they are based on monkey patching, which is generally frowned upon, for many good reasons.

Is this the recommended approach? Or is it purely by necessity? - For example upstream library do not want to accept adding more dependencies? (With log4j in recent memory this could be understandable) Maybe there are more reasons behind that I'm not aware of.

Personally i would think that keeping the instrumentation in the library main repository would ease maintenance and avoid incompatibilities arising from changes in the instrumented library. For basic decorators of public apis it's maybe not such a big risk, but ideally in-tree code will always be easier and safer to maintain than external patches. See https://github.com/open-telemetry/opentelemetry-python/discussions/1729 for an example of version conflict that could be avoided by keeping instrumentation in tree.


If OpenTelemetry is aiming to be universal, it should be an easy choice for a library to say that yes we want to support this. Compare it to logging, it is usually universal and kept in the main library together with the main source code, nobody does monkey-patching to add logging :) So rather than maintaining patches for all the libraries in the world here, should the first step be an attempt to upstream telemetry into the library? Taking this even further, linking to a rejected upstream attempt could even be a prerequisite before one is allowed to add a monkeypatch here?

I'm very happy to see so many popular libraries having instrumentation available via this repository, and guess it would not have been possible in such short time frame without patching, especially during the early stages of establishing reputation. My main question is more about the guidelines and best-practices one should follow when adding new instrumentation in future. Both when writing own libraries and when communicating with third party library maintainers.

owais commented 2 years ago

The ideal outcome would be for library maintainers to implement OpenTelemetry out of the box but we do not have the resources right now to go out and evangelize. There has interest from some projects (e.g, FastAPI).

If you are a library author, I would highly encourage you to add support for OpenTelemetry upstream. We'd be more than happy to answer any question and guide the implementation.

So rather than maintaining patches for all the libraries in the world here, should the first step be an attempt to upstream telemetry into the library? Taking this even further, linking to a rejected upstream attempt could even be a prerequisite before one is allowed to add a monkeypatch here?

Individual contributes may try that and it might be worth the effort but I don't think we can mandate this as a project as it would dramatically slow down the process of getting useful instrumentation into the hands of our users.

ocelotl commented 2 years ago

Individual contributes may try that and it might be worth the effort but I don't think we can mandate this as a project as it would dramatically slow down the process of getting useful instrumentation into the hands of our users.

Yes, this. Contributors may do so if they prefer but making it a requirement may put obstacles on them that we can't help overcome.