Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

confluent-kafka-python instrumentation #905

Conversation

Krishna-Vishwakarma
Copy link

@Krishna-Vishwakarma Krishna-Vishwakarma commented Feb 7, 2022

Description

added confluent-kafka-python module instrumentation

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

Added the following unit-tests:

  • instrumentation/opentelemetry-instrumentation-kafka-python/tests/test_instrumentation.py

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@Krishna-Vishwakarma Krishna-Vishwakarma requested a review from a team February 7, 2022 09:22
@linux-foundation-easycla
Copy link

CLA Not Signed

@Krishna-Vishwakarma Krishna-Vishwakarma marked this pull request as draft February 7, 2022 09:28
@Krishna-Vishwakarma
Copy link
Author

I can see there is already available instrumentation for kafka-python in opentelemetry-instrumentation-kafka-python package, so I have created a new package opentelemetry-instrumentation-confluent-kafka-python same as that also changed all dependent files eg. tox.ini, README.md, bootstrap_gen.py, etc.
Since confluet-kafka-python dependents on C client librdkafka and it does not provide any python wrapper instead it provides C files (Producer.c, Consumer.c). When I am trying to use wrap_function_wrapper in init.py file it is failing with error : TypeError: can't set attributes of built-in/extension type 'cimpl.Producer'
Example code:

wrap_function_wrapper(
            Producer, "produce", _wrap_send(tracer, produce_hook)
        )
wrap_function_wrapper(
    Consumer,"poll", _wrap_poll(tracer, consume_hook),
)

My Approach: I am monkey patching and adding new functions to class at runtime using forbiddenfruit library like below
from forbiddenfruit import curse

curse(Producer, "trace_produce", _wrap_produce(tracer, produce_hook))
curse(Consumer, "trace_poll", _wrap_poll(tracer, consume_hook))

It is working by using this approach but here we have to provide our own function(trace_produce and trace_poll ) and we can not use the actual produce and poll function of confluent-kafka because it is going in recursion as we have to invoke same method inside wrapper function.
My Question: Is there any other good way to do this.

2nd Problem: When I am executing test command tox -e test-instrumentation-confluent-kafka-python It is also failing with below error
========================================================================================================== no tests ran in 0.06s ===========================================================================================================
ERROR: InvocationError for command /Users/krishnakumarvishwakarma/Haystack/opentelemetry-confluent-kafka-python/opentelemetry-python-contrib/.tox/test-instrumentation-confluent-kafka-python/bin/pytest (exited with code 5)
_________________________________________________________________________________________________________________ summary __________________________________________________________________________________________________________________
ERROR: test-instrumentation-confluent-kafka-python: commands failed

3rd Problem: When I am running this test command it is generating one folder "test instrumentation/opentelemetry-instrumentation-kafka-python" inside my package "opentelemetry-instrumentation-confluent-kafka-python" it seems I missed some somewhere and it is still picking kafka's folder name
Where did I miss?

@owais
Copy link
Contributor

owais commented Feb 7, 2022

I am monkey patching and adding new functions to class at runtime using forbiddenfruit library like

We'll have to look at this library and see if we are fine with using it as nothing else uses it today AFAIK

@owais
Copy link
Contributor

owais commented Feb 7, 2022

It is working by using this approach but here we have to provide our own function(trace_produce and trace_poll ) and we can not use the actual produce and poll function of confluent-kafka because it is going in recursion as we have to invoke same method inside wrapper function.

Not sure I follow. Could you please elaborate this? It looks like you are calling the instance. from the instrumented library.

@Krishna-Vishwakarma
Copy link
Author

It is working by using this approach but here we have to provide our own function(trace_produce and trace_poll ) and we can not use the actual produce and poll function of confluent-kafka because it is going in recursion as we have to invoke same method inside wrapper function.

Not sure I follow. Could you please elaborate this? It looks like you are calling the instance. from the instrumented library.

Since confluet-kafka-python dependents on C client librdkafka and it does not provide any python wrapper, instead it provides C files (Producer.c, Consumer.c). When I am trying to use wrap_function_wrapper in init.py file to wrap actual Producer.produce() and Consumer.poll() funcitons with our custome function(which should internally generate traces and also call actual produce or poll function )it is failing with error : TypeError: can't set attributes of built-in/extension type 'cimpl.Producer'

As I am not able to wrap native fuction with our custom function, I found another way to ingest our custom fuction(trace_produce and trace_poll) in Producer and Consumer class at runtime to allow Python code to extend built-in types I am using forbiddenfruit library.

Working code which allow to ingest our custom function to existing Producer and Consumer class
from forbiddenfruit import curse

curse(Producer, "trace_produce", _wrap_produce(tracer, produce_hook))
curse(Consumer, "trace_poll", _wrap_poll(tracer, consume_hook))

But by this approach we are providing our function trace_produce and trace_consume so any application developer has to call these methods instead of actual Producer.produce and Consumer.poll
Is this a good approach I am asking?

@Krishna-Vishwakarma
Copy link
Author

2nd Problem: When I am executing test command tox -e test-instrumentation-confluent-kafka-python It is also failing with below error
========================================================================================================== no tests ran in 0.06s ===========================================================================================================
ERROR: InvocationError for command /Users/krishnakumarvishwakarma/Haystack/opentelemetry-confluent-kafka-python/opentelemetry-python-contrib/.tox/test-instrumentation-confluent-kafka-python/bin/pytest (exited with code 5)
_________________________________________________________________________________________________________________ summary __________________________________________________________________________________________________________________
ERROR: test-instrumentation-confluent-kafka-python: commands failed

@Krishna-Vishwakarma
Copy link
Author

3rd Problem: When I am running this test command it is generating one folder "test instrumentation/opentelemetry-instrumentation-kafka-python" inside my package "opentelemetry-instrumentation-confluent-kafka-python" it seems I missed some somewhere and it is still picking kafka's folder name
Where did I miss?

except Exception as hook_exception: # pylint: disable=W0703
_LOG.exception(hook_exception)

return instance.produce(*args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be inside the span context?

@owais
Copy link
Contributor

owais commented Feb 8, 2022

But by this approach we are providing our function trace_produce and trace_consume so any application developer has to call these methods instead of actual Producer.produce and Consumer.poll

Oh, now I get it. No, this wouldn't be OK for an auto-instrumentation package as it does not automatically instrument anything i.e, people have to modify every call site in their projects to trace kafka calls. It sounds more like manual instrumentation with some helpers.

@Krishna-Vishwakarma
Copy link
Author

But by this approach we are providing our function trace_produce and trace_consume so any application developer has to call these methods instead of actual Producer.produce and Consumer.poll

Oh, now I get it. No, this wouldn't be OK for an auto-instrumentation package as it does not automatically instrument anything i.e, people have to modify every call site in their projects to trace kafka calls. It sounds more like manual instrumentation with some helpers.

Can you please give some pointer/suggesion on other possible approaches to instrument packages which uses native c functions in python, Since confluet-kafka-python dependents on C client librdkafka and it does not provide any python wrapper

@srikanthccv
Copy link
Member

@oxeye-dorkolog oxeye-dorkolog mentioned this pull request May 31, 2022
7 tasks
@lzchen
Copy link
Contributor

lzchen commented Jun 2, 2022

@Krishna-Vishwakarma
Are you still working on this PR?

@srikanthccv
Copy link
Member

@Krishna-Vishwakarma the pr #1111 added support for confluent kafka. Is there anything left to do? And are you still interested in continuing this work?

@srikanthccv
Copy link
Member

@Krishna-Vishwakarma I am going to close this since there hasn't been any activity. Feel free to re-open and rework if the other PR which added confluent kafka doesn't solve your problems.

@srikanthccv srikanthccv closed this Jul 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants