Instrumenting Redis by codeboten · Pull Request #21 · open-telemetry/opentelemetry-python-contrib

codeboten · 2020-04-04T05:36:15Z

Porting the existing redis instrumentation to using the OpenTelemetry API and the OpenTelemetry Auto-instrumentation Patcher interface.

The process to make this happen is pretty straight forward:

move the redis code into the instrumentors directory
move tests
added setup/version files and move code into a similar directory structure to other opentelemetry integrations
fixed relative import paths to using ddtrace package
update the code to use opentelemetry API
update the tests
add tests to tox

Couple of TODOs left:

the original redis instrumentation set a couple of metrics in the metadata, this could be a good opportunity to use the metrics API
pin versions of API & SDK

NOTE: This currently still uses some utility functions in the ddtrace package.

fixes #15

docker-compose.yml

pytest.ini

ocelotl · 2020-04-10T17:50:02Z

instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py

+        assert span.attributes["service"] == self.TEST_SERVICE
+
+    def test_patch_unpatch(self):
+        # Test patch idempotence


opentelemetry.instrumentor.BaseInstrumentor.instrument already takes care of idempotence. It would be better to move the code inside patch to _instrument in order to be able to benefit from this feature.

it makes sense to test the idempotency of patch/unpatch for anyone using it to programmatically enable instrumentation

...ntation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/patch.py

...ion/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/constants.py

...entation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py

...ion/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/constants.py

mauriciovasquezbernal

Some more minor comments, I tested and it works with a basic example.

I haven't reviewed the exact attributes you are setting on the span, as those come from datadog I'll suppose they are fine.

instrumentation/opentelemetry-instrumentation-redis/setup.cfg

instrumentation/opentelemetry-instrumentation-redis/setup.py

mauriciovasquezbernal · 2020-04-15T15:42:22Z

...tion/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py

+    import redis
+
+    # You can patch redis specifically
+    patch()


I strongly think this should be using also the instrumentor interface, but will ignore for now until we clarify those ideas.

I agree, I'd rather not block here until the auto-instrumentation interface is completed. Here's an issue to track this: #38

mauriciovasquezbernal · 2020-04-15T17:00:34Z

...ntation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/patch.py

+
+
+def traced_execute_command(func, instance, args, kwargs):
+    tracer = trace.get_tracer(_DEFAULT_SERVICE, __version__)


I think this need some more discussion, creating a new tracer for each operation doesn't look like a good solution to me. Somehow the integration should have access to a tracer and avoid creating it for each operation call. I opened an issue for a related discussion open-telemetry/opentelemetry-python#585.

mauriciovasquezbernal · 2020-04-15T17:07:27Z

instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py

+            pipeline.execute()
+
+        spans = self.get_spans()
+        self.assertEqual(len(spans), 2)


Out of curiosity, what is the second span and why there is not any test on that?

added a comment to clarify, also updated the test to use different set parameters

instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py

Co-Authored-By: Mauricio Vásquez <mauricio@kinvolk.io>

mauriciovasquezbernal

LGTM, just a minor comment that could be implemented in further PRs.

mauriciovasquezbernal · 2020-04-16T19:48:54Z

...ntation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/patch.py

+_CMD = "redis.command"
+
+
+def patch(tracer=None):


Maybe it's a good idea to update right now to take a TracerProvider as described in open-telemetry/opentelemetry-python#585. Up to you.

codeboten · 2020-04-17T16:36:02Z

Closing in favour of open-telemetry/opentelemetry-python#595

Porting the existing redis instrumentation from the contrib repo to using the OpenTelemetry API and the OpenTelemetry Auto-instrumentation Instrumentor interface. Similiar to the sqlalchemy PR, the main thing that will need updating is to remove the patch/unpatch methods once the instrumentor interface changes have been merged. This is replacing open-telemetry/opentelemetry-python-contrib#21 Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io> Co-authored-by: Leighton Chen <lechen@microsoft.com> Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com> Co-authored-by: Chris Kleinknecht <libc@google.com>

…y#595) Porting the existing redis instrumentation from the contrib repo to using the OpenTelemetry API and the OpenTelemetry Auto-instrumentation Instrumentor interface. Similiar to the sqlalchemy PR, the main thing that will need updating is to remove the patch/unpatch methods once the instrumentor interface changes have been merged. This is replacing open-telemetry#21 Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io> Co-authored-by: Leighton Chen <lechen@microsoft.com> Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com> Co-authored-by: Chris Kleinknecht <libc@google.com>

Alex Boten added 6 commits April 3, 2020 15:44

move redis directory

7262fbb

move tests

b3cb5c5

create opentelemetry-ext-redis package

a11a593

fix import paths

9d63b7e

update code to use opentelemetry API

a0e9498

removing legacy code

2b38380

codeboten requested review from mauriciovasquezbernal and ocelotl April 4, 2020 05:36

alrex added 2 commits April 3, 2020 22:46

Update setup.cfg

4c2cfea

fix tests

38daba2

codeboten marked this pull request as ready for review April 7, 2020 20:04

fix naming to use instrumentation

514ea82

codeboten requested a review from a team April 9, 2020 18:24

Alex Boten added 14 commits April 9, 2020 11:26

Merge remote-tracking branch 'origin/master' into instrumentor-redis

ca0ba2e

cleaning up code

f669783

Merge remote-tracking branch 'origin/master' into instrumentor-redis

a6c2626

more cleanup

bcd8d76

fix readme

064c33a

fixing tox, ignore reference for pytest

6500779

adding docker compose file

aa19ed4

cleanup tox, add dep for py34

fdac399

adding constraint for ddtrace version

a9c866e

fix dep

ceea9d1

fix isort

7d9883d

removing some dependency on ddtrace

7d8fbcb

removing dep on ddtrace

aff183f

remove from requirements

40aa813

ocelotl suggested changes Apr 10, 2020

View reviewed changes

Alex Boten added 2 commits April 10, 2020 15:03

review cleanup

dea67fe

set attributes for what used to be metric

e3d35c7

Alex Boten added 8 commits April 14, 2020 10:41

set versions

f1c1702

updating docs

98b4475

removing comment

591f4d7

revert change to use full query in span name, more feedback fixes

3b63eff

updating docs

cb3030e

sdk is not required

e294fbc

test still needs the sdk

e96f43c

fix missing dep

676c349

mauriciovasquezbernal reviewed Apr 15, 2020

View reviewed changes

Alex Boten and others added 8 commits April 15, 2020 14:28

pin otel dependencies

c53e2aa

move entrypoint to setup.cfg

b63e7c2

use semantic conventions, remove useless test

f71f885

Update instrumentation/opentelemetry-instrumentation-redis/setup.cfg

18a847f

Co-Authored-By: Mauricio Vásquez <mauricio@kinvolk.io>

cleaning up formatting method, updating test

8e1b66e

don't get a tracer on each operation

ae4def8

updating test

18c247a

method was used in the context of Pin

798d655

codeboten requested review from mauriciovasquezbernal and ocelotl April 16, 2020 15:47

mauriciovasquezbernal approved these changes Apr 16, 2020

View reviewed changes

codeboten mentioned this pull request Apr 17, 2020

Porting redis instrumentation from contrib repo open-telemetry/opentelemetry-python#595

Merged

codeboten closed this Apr 17, 2020

NathanielRN pushed a commit to NathanielRN/opentelemetry-python-contrib-1 that referenced this pull request Oct 17, 2020

Move extant tracing API into new pkg (open-telemetry#21)

41f0caa

codeboten deleted the instrumentor-redis branch November 24, 2020 17:05



		def traced_execute_command(func, instance, args, kwargs):
		tracer = trace.get_tracer(_DEFAULT_SERVICE, __version__)

Conversation

codeboten commented Apr 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codeboten commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codeboten commented Apr 4, 2020 •

edited

Loading