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.

Sign up for GitHub

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

Jump to bottom

kafka-python instrumentation #814

Merged
merged 24 commits into from
Jan 10, 2022
Merged

kafka-python instrumentation #814

merged 24 commits into from
Jan 10, 2022

Conversation

ItayGibel-helios
Copy link
Contributor

Description

added 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
  • instrumentation/opentelemetry-instrumentation-kafka-python/tests/test_utils.py

Does This PR Require a Core Repo Change?

  • No.

Checklist:

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

@ItayGibel-helios ItayGibel-helios requested a review from a team November 20, 2021 11:13
owais
owais reviewed Nov 20, 2021
View reviewed changes
.../opentelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/utils.py
span.set_attribute(SpanAttributes.MESSAGING_DESTINATION, topic)
span.set_attribute(SpanAttributes.MESSAGING_KAFKA_PARTITION, partition)
span.set_attribute(
SpanAttributes.MESSAGING_URL, json.dumps(bootstrap_servers)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a plain URL, why json.dumps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

boostrap_servers is a list of URLs

.../opentelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/utils.py Outdated Show resolved Hide resolved
.../opentelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/utils.py Outdated Show resolved Hide resolved
@lzchen
Copy link
Contributor

lzchen commented Dec 7, 2021

@ItayGibel-helios
Thanks for the contribution. Have you gotten a chance to take a look at the contributing guidelines and expectations? If this PR gets merged, are you okay with being a maintainer for this instrumentation?

lzchen
lzchen reviewed Dec 7, 2021
View reviewed changes
...pentelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/package.py
# limitations under the License.


_instruments = ("kafka-python >= 2.0",)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, what is the difference between kafka and kafka-python? Will this only instrument kafka-python?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they are the same project, but newer versions are released as kafka-python (latest 'kafka' release was in 2017)

Copy link
Contributor

Choose a reason for hiding this comment

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

Curious should we rename all instances of kafka to kafka-python then? e.g. import path, tox environments, entrypoint, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to keep with the kafka library conventions, where the package name is kafka-python but you actually use kafka in the import paths

@ItayGibel-helios
Copy link
Contributor Author

@ItayGibel-helios Thanks for the contribution. Have you gotten a chance to take a look at the contributing guidelines and expectations? If this PR gets merged, are you okay with being a maintainer for this instrumentation?

Hi @lzchen
I've worked on this PR together with @nozik and he is gonna be the maintainer of this instrumentation once merged

@oxeye-nikolay oxeye-nikolay removed the request for review from nikosokolik December 27, 2021 12:30
ashu658 and others added 12 commits December 28, 2021 17:08
@ashu658 @ItayGibel-helios
Conditionally create server spans for flask (open-telemetry#828)
6eb3259 
* Making span as internal in presence of a span in current context

* Updating changelog

* Removing extra print statements

* Resolving comments: Setting current context as parent in its presence

* Ignoring pylint check as django.conf.urls.url is removed in django 4.0
Django release notes: https://docs.djangoproject.com/en/4.0/releases/4.0/

* Removing changes in django files
@srikanthccv @ItayGibel-helios
Fix CHANGELOG (open-telemetry#839)
abb4f28
@ItayGibel-helios
kafka-python instrumentation
0589ea9
@ItayGibel-helios
tests
0e417db
@ItayGibel-helios
__next__ instrumeentation
680d7d2
@ItayGibel-helios
update version
40cb626
@ItayGibel-helios
lint
1d9b903
@ItayGibel-helios
replace dummy hook with callable check, add hook documentation
8d40122
@ItayGibel-helios
genreate
96d7b88
@ItayGibel-helios
create_consumer_span function
77197dc
@ItayGibel-helios
update kafka dependency version
8bddbec
@ItayGibel-helios
Fix README
63acbdb
lzchen
lzchen reviewed Jan 3, 2022
View reviewed changes
tox.ini Outdated Show resolved Hide resolved
@ItayGibel-helios @lzchen
Update tox.ini
0c26d50 
Co-authored-by: Leighton Chen <[email protected]>
@ocelotl
Copy link
Contributor

ocelotl commented Jan 7, 2022

@lzchen @nikosokolik This PR can be merged now, is there any pending comment yet?

@lzchen
Copy link
Contributor

lzchen commented Jan 7, 2022

@oxeye-nikolay
Any more issues with this PR? If not can we get an approval?

@ItayGibel-helios
Please merge with the latest main.

nikosokolik
nikosokolik approved these changes Jan 8, 2022
View reviewed changes
Copy link

@nikosokolik nikosokolik left a comment

Choose a reason for hiding this comment

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

This is @oxeye-nikolay. Using this account to approve instead.

@lzchen lzchen merged commit e67a728 into open-telemetry:main Jan 10, 2022
lzchen added a commit that referenced this pull request Jan 10, 2022
@lzchen
Add @nozik as maintainer of kafka-python instrumentation
9a813ed 
From this comment #814 (comment)
@lzchen lzchen mentioned this pull request Jan 10, 2022
Merged
@nozik nozik deleted the kafka branch April 11, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxeye-nikolay oxeye-nikolay oxeye-nikolay left review comments

@nikosokolik nikosokolik nikosokolik approved these changes

@lzchen lzchen lzchen approved these changes

@owais owais Awaiting requested review from owais

Assignees

@ItayGibel-helios ItayGibel-helios

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@ItayGibel-helios @lzchen @ocelotl @owais @nikosokolik @oxeye-nikolay @kuba-wu @ashu658 @srikanthccv @nozik