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

kafka-python-ng: Instrument temporary fork kafka-python-ng by copying kafka-python's instrumentation #2532

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rjduffner
Copy link
Contributor

@rjduffner rjduffner commented May 20, 2024

Description

This PR creates a new kafka-python-ng instrumentation to instrument the new (and temporary) fork of kafka-python called kafka-python-ng (https://github.com/wbarnha/kafka-python-ng). It copies all of the instrumentation from the kafka-python instrumentation and places it in the new folder.

The old maintainer of kafka-python is no longer able to support the project and has passed the project off to a new maintainer. However, the new maintainer does not yet have access to the kafka-python namespace in pypi so cannot release a new version. See dpkp/kafka-python#2431

Since this fork looks to be semi temporary, I think its a little strange to create a whole new instrumentation for it but the issue linked above gives no timeline on when we might expect the reunification.

I originally tried to use the old instrumentation and just point it at either kafka-python or kafka-python-ng but it didn't seemed to treat comma delineated packages as ANDs instead of ORs. If there is a way to say, "this package or that package" I think that might be a better solution but I did not see any examples of it.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tested locally. Same tests as the previous kafka-python instrumentation

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

@rjduffner rjduffner requested a review from a team as a code owner May 20, 2024 17:55
@rjduffner rjduffner changed the title kafka-python: Instrument temporary fork kafka-python-ng alongside kafka-python [WIP] kafka-python: Instrument temporary fork kafka-python-ng alongside kafka-python May 20, 2024
@rjduffner rjduffner force-pushed the kafka-python-ng-add-rd branch 3 times, most recently from adce71c to 6b4cd0e Compare May 20, 2024 20:27
@rjduffner rjduffner changed the title [WIP] kafka-python: Instrument temporary fork kafka-python-ng alongside kafka-python kafka-python-ng: Instrument temporary fork kafka-python-ng by copying kafka-python's instrumentation May 20, 2024
@xrmx
Copy link
Contributor

xrmx commented May 21, 2024

Yeah, I think it would be better to fix the ability to wrap the same code for more than one package instead of publishing duplicated or even temporary packages.

@rjduffner
Copy link
Contributor Author

@xrmx, ya i think the same thing. And it may not be too hard to do if the packages installed in seperate namespaces/import paths.

As they sit now both packages (kafka-python and kafka-python-ng) both use "import kafka". So in the init.py where I would presumably hook in here, https://github.com/rjduffner/opentelemetry-python-contrib/blob/kafka-python-ng-add-rd/instrumentation/opentelemetry-instrumentation-kafka-python-ng/src/opentelemetry/instrumentation/kafka/__init__.py#L91-L122 I can't differentiate (as far as I know) between the two packages.

While the case where one or the other is installed is fine, if the user has them both installed by accident, this may lead to strange results.

@rjduffner
Copy link
Contributor Author

Second stab at the original idea of instrument either kafka-python or kafka-python-ng here #2537

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.

None yet

2 participants