open-telemetry / open-telemetry/opentelemetry-python-contrib
Falcon monkey patching fails with subclassed apps or wrong import order
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 1.1k
- Forks
- 1.1k
- Avg merge
- 4d 15h
- Merged PRs (30d)
- 16
Description
This could be considered a bug, poorly documented behavior or just fragility of monkey patching.
import falcon
# in app.py or something
class MyApp(falcon.App):
...
# in main.py or something
import falcon
from falcon import App
from opentelemetry.instrumentation.falcon import FalconInstrumentor
from app import MyApp
FalconInstrumentor.instrument()
App() # won't be instrumented
MyApp() # won't be instrumented
falcon.App() # will be instrumented
This can be fixed by instrumenting before from falcon import App and from app import MyApp:
import falcon
# in app.py or something
class MyApp(falcon.App):
...
# in main.py or something
import falcon
from opentelemetry.instrumentation.falcon import FalconInstrumentor
FalconInstrumentor.instrument()
from falcon import App
from app import MyApp
App() # won't be instrumented
MyApp() # won't be instrumented
falcon.App() # will be instrumented
But:
- Linters won't be happy
- This is super ugly
- The only way to figure this out is to dig deep into the OpenTelemetry source code, understand where/how the monkey patching is happening and then apply the fix. It's not exactly documented.
I understand the desire to provide users with "auto-instrumentation" by means a single function call / even running opentelemetry-instrument on unmodified code. It's super cool! But it can't be the only way to do things. As demonstrated here there are a lot of rough edges to this.
It would be nice if the API was presented in a layered fashion:
opentelemetry-instrumentFalconInstrumentor.instrument()- Falcon Middleware + WSGI middleware that just calls some public methods / uses a public context manager.
- Falcon Middleware + the public context manager
This way, users can just go with opentelemtry-instrument if that works for them. But if that doesn't work, they have the option to peel back the onion, read a bit deeper into the docs and understand how to add the middleware themselves and such. This also decreases the burden of functionality of auto-instrumentation: if in certain situations it is too hard / fragile / hacky to get something working via auto-instrumentation, but it is possible with a bit more work (e.g. a WSGI wrapper), this can be documented as "if you want XYZ advanced feature, you can instrument yourself and configure it following this example".
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with FalconInstrumentor.instrument() and the opentelemetry-instrument entry point, then trace how Falcon.App and subclasses are monkey-patched relative to import order. Review the Falcon middleware and WSGI middleware options described in the issue. Done means the supported instrumentation layers and their import or configuration requirements are implemented or documented clearly, including a path that works for subclassed apps.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, observability
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100