fix(instrumentation-confluent-kafka): add __getattr__ delegation to ProxiedProducer and ProxiedConsumer#4648
Conversation
|
|
3e3d45b to
2440029
Compare
…roxiedProducer and ProxiedConsumer ProxiedProducer and ProxiedConsumer only forward an explicit list of methods to the underlying confluent-kafka objects. Calling any method outside that list (e.g. list_topics(), assignment(), memberid(), set_sasl_credentials()) raises AttributeError on pure-Python mocks or causes a segmentation fault on real confluent-kafka C-extension objects, because the C-extension Producer/Consumer base classes don't implement standard Python attribute lookup. Add __getattr__ to both proxy classes so that any attribute or method lookup not satisfied by an explicitly defined proxy method is forwarded to the underlying _producer/_consumer via getattr(). This is the standard Python delegation pattern and has no impact on methods that are already explicitly proxied. Fixes open-telemetry#4278 Assisted-by: Claude Sonnet 4.5
2440029 to
474a586
Compare
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
github-project-automation
Bot
moved this from Ready for review
to Reviewed PRs that need fixes
in Python PR digest
|
Makes sense — I'll switch this to wrapt.ObjectProxy instead of getattr. |
… classes ProxiedProducer and ProxiedConsumer forwarded only an explicit list of methods to the underlying confluent-kafka objects, so any method outside that list (list_topics(), assignment(), memberid(), set_sasl_credentials()) raised AttributeError on pure-Python mocks or segfaulted on the real C-extension objects. Base both proxies on wrapt.ObjectProxy, which transparently forwards every attribute and method that is not explicitly overridden. This removes the hand-maintained pass-through list (flush, poll, purge, committed, commit, get_watermark_offsets, offsets_for_times, subscribe) along with the need for manual __getattr__ delegation, leaving only the instrumented methods (produce, poll, consume, close). Proxy-local state uses ObjectProxy's _self_ storage. The two attributes the consume-tracking helpers in utils.py read and write on the instance (_current_consume_span, _current_context_token) are exposed as properties backed by that storage, so the state stays on the proxy instead of being forwarded onto the wrapped C-extension client. Tests that asserted the exact proxy class now use assertIsInstance, since ObjectProxy intentionally reports the wrapped object's __class__.
|
Switched to wrapt.ObjectProxy — thanks for the pointer, it's cleaner than the getattr approach. Since ObjectProxy forwards anything not explicitly overridden, I was able to drop the whole hand-maintained pass-through list (flush, poll, purge, committed, commit, get_watermark_offsets, offsets_for_times, subscribe) — the proxies now only define the instrumented methods. Net -75/+72. Two things worth flagging though ( assuming while youre doing diff might be useful): utils.py reads and writes _current_consume_span / _current_context_token on the instance, so those are properties backed by ObjectProxy's self storage. Otherwise they'd get forwarded onto the wrapped consumer, which has no dict. |
|
|
||
|
|
||
| class ProxiedProducer(Producer): | ||
| class ProxiedProducer(wrapt.ObjectProxy): |
There was a problem hiding this comment.
Please use BaseObjectProxy if available becase ObjectProxy always implementes __iter__ even if the proxied object doesn't, we do this in other instrumentations:
try:
# wrapt 2.0.0+
from wrapt import BaseObjectProxy
except ImportError:
from wrapt import ObjectProxy as BaseObjectProxy
| # The consume-tracking helpers in utils.py read and write these two | ||
| # attributes on the instance. Back them with ObjectProxy's `_self_` | ||
| # storage via properties so the state lives on the proxy rather than being | ||
| # forwarded onto the wrapped consumer, whose C-extension type does not | ||
| # accept arbitrary attributes. | ||
| @property | ||
| def _current_consume_span(self): | ||
| return self._self_current_consume_span | ||
|
|
||
| @_current_consume_span.setter | ||
| def _current_consume_span(self, value) -> None: | ||
| self._self_current_consume_span = value | ||
|
|
||
| @property | ||
| def _current_context_token(self): | ||
| return self._self_current_context_token | ||
|
|
||
| @_current_context_token.setter | ||
| def _current_context_token(self, value) -> None: | ||
| self._self_current_context_token = value |
There was a problem hiding this comment.
I don't see much value on these wrappers, I think we can live with the caller referencing the ._self_ version directly
| self.assertEqual(consumer.__class__, ProxiedConsumer) | ||
| # See test_instrument_api: ObjectProxy reports the wrapped __class__. | ||
| self.assertIsInstance(consumer, ProxiedConsumer) | ||
| # commit() is no longer declared on the proxy; ObjectProxy forwards it. |
There was a problem hiding this comment.
| # commit() is no longer declared on the proxy; ObjectProxy forwards it. |
Looks more like a changelog
3 participants
Description
Fixes #4278
ProxiedProducerandProxiedConsumeronly forward an explicit set of methods to the underlying confluent-kafka objects. Calling any method outside that list — such aslist_topics(),assignment(),memberid(), orset_sasl_credentials()— raisesAttributeErroron pure-Python mocks or causes a segmentation fault on real confluent-kafka C-extension objects, because the C-extension base classes don't implement standard Python attribute lookup.Add
__getattr__to both proxy classes so that any attribute or method lookup not satisfied by an explicitly defined proxy method is forwarded to the underlying_producer/_consumerviagetattr(). Methods already explicitly proxied are unaffected.Type of change
How Has This Been Tested?
Two new unit tests added to
tests/test_instrumentation.py:test_proxied_producer_delegates_unproxied_methods— verifiesProxiedProducerforwards a method not in its explicit proxy list via__getattr__test_proxied_consumer_delegates_unproxied_methods— same forProxiedConsumerDoes This PR Require a Core Repo Change?