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 Instrument temporary fork, kafka-python-ng inside kafka-python's instrumentation #2537

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

rjduffner
Copy link
Contributor

@rjduffner rjduffner commented May 21, 2024

Description

This PR adds kafka-python-ng, a temporary fork of kafka-python due to pypi access issues, to the kafka-python instrumentation.

Type of change

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

How Has This Been Tested?

  • Test 1: Setup the code locally and then made sure that a sample pyramid app using kafka-python-ng instrumented without any errors.

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 May 21, 2024 15:37
@rjduffner rjduffner changed the title [WIP] kafka-python Instrument temporary fork, kafka-python-ng inside kafka-python's instrumentation kafka-python Instrument temporary fork, kafka-python-ng inside kafka-python's instrumentation May 21, 2024
@rjduffner
Copy link
Contributor Author

@xrmx, take a look at this one. I still need to do some doc updates but I think this is a better option vs #2537

@xrmx
Copy link
Contributor

xrmx commented May 27, 2024

@rjduffner My preferred approach would be something like this but unfortunately we are using a field with the semantics already defined (must be strings) #2409

Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

I think would also be nice to have kafka-python-ng in a test-requirements file to check we support it on tests as well

@rjduffner rjduffner force-pushed the kafka-python-ng-add-rd-2 branch 2 times, most recently from 26b786c to 6ebb04c Compare July 31, 2024 20:06
@rjduffner
Copy link
Contributor Author

@emdneto, it looks like fastapi and fastapi-slim do something similar

  fastapi: pip install opentelemetry-semantic-conventions@{env:CORE_REPO}\#egg=opentelemetry-semantic-conventions&subdirectory=opentelemetry-semantic-conventions
  fastapi: pip install opentelemetry-sdk@{env:CORE_REPO}\#egg=opentelemetry-sdk&subdirectory=opentelemetry-sdk
  fastapi: pip install opentelemetry-test-utils@{env:CORE_REPO}\#egg=opentelemetry-test-utils&subdirectory=tests/opentelemetry-test-utils
  fastapi: pip install -r {toxinidir}/instrumentation/opentelemetry-instrumentation-fastapi/test-requirements.txt
  fastapi-slim: pip install opentelemetry-api@{env:CORE_REPO}\#egg=opentelemetry-api&subdirectory=opentelemetry-api
  fastapi-slim: pip install opentelemetry-semantic-conventions@{env:CORE_REPO}\#egg=opentelemetry-semantic-conventions&subdirectory=opentelemetry-semantic-conventions
  fastapi-slim: pip install opentelemetry-sdk@{env:CORE_REPO}\#egg=opentelemetry-sdk&subdirectory=opentelemetry-sdk
  fastapi-slim: pip install opentelemetry-test-utils@{env:CORE_REPO}\#egg=opentelemetry-test-utils&subdirectory=tests/opentelemetry-test-utils
  fastapi-slim: pip install -r {toxinidir}/instrumentation/opentelemetry-instrumentation-fastapi/test-requirements-slim.txt

Ive already copied what they are doing for the instruments part, ill copy them for the tests too. Thanks for the suggestion.

Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

@rjduffner It seems we have a problem with the way fastapi-slim support was implemented, so hopefully we can use the same fix here. See #2756

@rjduffner
Copy link
Contributor Author

rjduffner commented Jul 31, 2024

@emdneto

Ha, I just saw that and was fighting with bugs in the tests and switched my code back to doing it the way the fix for fastAPI is doing it. Great timing

@rjduffner
Copy link
Contributor Author

@xrmx and @emdneto

Thanks for the feedback. I think it is all integrated at this time.

tox.ini Outdated Show resolved Hide resolved
@rjduffner
Copy link
Contributor Author

@xrmx, I updated the tox to switch everything to kafka-pythonng from kafka-python-ng.

However, now the action only installs the kafka-python-ng instrumentation and doesn't actually test with it because I didn't update these to test kafka-python-ng side

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/tox.ini#L937-L941

I saw that fastapislim didn't update those but that seems like an oversight. Im guessing we want a section here for the kafka-python-ng tests that would be identical to the kafka-python ones.

@emdneto
Copy link
Member

emdneto commented Aug 1, 2024

@rjduffner, you're right; the job isn't running the test. I will submit a fix.

@xrmx, I updated the tox to switch everything to kafka-pythonng from kafka-python-ng.

However, now the action only installs the kafka-python-ng instrumentation and doesn't actually test with it because I didn't update these to test kafka-python-ng side

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/tox.ini#L937-L941

I saw that fastapislim didn't update those but that seems like an oversight. Im guessing we want a section here for the kafka-python-ng tests that would be identical to the kafka-python ones.

@rjduffner
Copy link
Contributor Author

@emdneto cool, ill do the same here. Thanks for the confirmation.

@ocelotl ocelotl merged commit 265490f into open-telemetry:main Aug 2, 2024
395 checks passed
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