kafka-python instrumentation by ItayGibel-helios · Pull Request #814 · open-telemetry/opentelemetry-python-contrib

ItayGibel-helios · 2021-11-20T11:13:35Z

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

owais · 2021-11-20T12:34:07Z

.../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)


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

boostrap_servers is a list of URLs

.../opentelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/utils.py

...entelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/__init__.py

.../opentelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/utils.py

lzchen · 2021-12-07T15:37:55Z

@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?

instrumentation/opentelemetry-instrumentation-kafka-python/README.rst

lzchen · 2021-12-07T15:43:48Z

...pentelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/package.py

+# limitations under the License.
+
+
+_instruments = ("kafka-python >= 2.0",)


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

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

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

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

...entelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/__init__.py

.../opentelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/utils.py

ItayGibel-helios · 2021-12-07T19:15:57Z

@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

…DME.rst Co-authored-by: Leighton Chen <lechen@microsoft.com>

* 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

tox.ini

Co-authored-by: Leighton Chen <lechen@microsoft.com>

ocelotl · 2022-01-07T21:08:04Z

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

lzchen · 2022-01-07T23:08:05Z

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

@ItayGibel-helios
Please merge with the latest main.

nikosokolik

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

From this comment #814 (comment)

ItayGibel-helios requested a review from a team November 20, 2021 11:13

ItayGibel-helios force-pushed the kafka branch 6 times, most recently from 310d8bb to 79d4b77 Compare November 20, 2021 12:24

owais reviewed Nov 20, 2021

View reviewed changes

oxeye-nikolay reviewed Nov 21, 2021

View reviewed changes

ItayGibel-helios force-pushed the kafka branch 3 times, most recently from c40fceb to 0d1d010 Compare November 23, 2021 12:36

lzchen reviewed Dec 7, 2021

View reviewed changes

instrumentation/opentelemetry-instrumentation-kafka-python/README.rst Outdated Show resolved Hide resolved

lzchen reviewed Dec 7, 2021

View reviewed changes

...entelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/__init__.py Show resolved Hide resolved

lzchen reviewed Dec 7, 2021

View reviewed changes

.../opentelemetry-instrumentation-kafka-python/src/opentelemetry/instrumentation/kafka/utils.py Outdated Show resolved Hide resolved

github-actions bot assigned nikosokolik and oxeye-nikolay Dec 26, 2021

github-actions bot requested a review from nikosokolik December 26, 2021 18:58

oxeye-nikolay removed the request for review from nikosokolik December 27, 2021 12:30

oxeye-nikolay assigned ItayGibel-helios and unassigned nikosokolik and oxeye-nikolay Dec 27, 2021

ItayGibel-helios and others added 5 commits December 28, 2021 17:05

kafka-python instrumentation

4018020

Fixing span name

8719063

PR fixes

d184f36

Better context management

627f3c7

Update instrumentation/opentelemetry-instrumentation-kafka-python/REA…

132c9eb

…DME.rst Co-authored-by: Leighton Chen <lechen@microsoft.com>

ashu658 and others added 12 commits December 28, 2021 17:08

Fix CHANGELOG (open-telemetry#839)

abb4f28

kafka-python instrumentation

0589ea9

tests

0e417db

__next__ instrumeentation

680d7d2

update version

40cb626

lint

1d9b903

replace dummy hook with callable check, add hook documentation

8d40122

genreate

96d7b88

create_consumer_span function

77197dc

update kafka dependency version

8bddbec

Fix README

63acbdb

ItayGibel-helios force-pushed the kafka branch from 4059c59 to 63acbdb Compare December 28, 2021 15:10

ItayGibel-helios added 2 commits December 28, 2021 17:16

generate

5bbbf22

remove a changelog record that was duplicated by mistake

6d551ae

ItayGibel-helios force-pushed the kafka branch from 0486a98 to 6d551ae Compare December 28, 2021 15:29

fix typo

066d575

ItayGibel-helios requested review from lzchen and owais December 28, 2021 15:44

lzchen reviewed Jan 3, 2022

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

lzchen approved these changes Jan 3, 2022

View reviewed changes

Update tox.ini

0c26d50

Co-authored-by: Leighton Chen <lechen@microsoft.com>

Merge branch 'main' into kafka

e06f1e7

nikosokolik approved these changes Jan 8, 2022

View reviewed changes

lzchen merged commit e67a728 into open-telemetry:main Jan 10, 2022

lzchen added a commit that referenced this pull request Jan 10, 2022

Add @nozik as maintainer of kafka-python instrumentation

9a813ed

From this comment #814 (comment)

lzchen mentioned this pull request Jan 10, 2022

Add @nozik as maintainer of kafka-python instrumentation #849

Merged

nozik deleted the kafka branch April 11, 2022 14:01

		# limitations under the License.


		_instruments = ("kafka-python >= 2.0",)

Conversation

ItayGibel-helios commented Nov 20, 2021

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

Uh oh!

owais Nov 20, 2021

Choose a reason for hiding this comment

Uh oh!

ItayGibel-helios Nov 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lzchen commented Dec 7, 2021

Uh oh!

Uh oh!

lzchen Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

ItayGibel-helios Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

lzchen Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

ItayGibel-helios Dec 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ItayGibel-helios commented Dec 7, 2021

Uh oh!

Uh oh!

ocelotl commented Jan 7, 2022

Uh oh!

lzchen commented Jan 7, 2022

Uh oh!

nikosokolik left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants