Porting redis instrumentation from contrib repo #595

codeboten · 2020-04-17T16:34:48Z

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

mauriciovasquezbernal

Very minor details.
I only concerned about the functional tests, I'm not 100% convinced we should we all functional testing in the same place.

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

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

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

tox.ini

mauriciovasquezbernal · 2020-04-20T14:04:52Z

ext/opentelemetry-ext-docker-tests/tests/docker-compose.yml

@@ -23,4 +23,8 @@ services:
      POSTGRES_USER: testuser
      POSTGRES_PASSWORD: testpassword
      POSTGRES_DB: opentelemetry-tests
+  otredis:


I'm wondering if it is a really good approach to keep the tests of the different frameworks under the same test. What about if I only want to test redis?
Is there a technical limitation to do that?

there's no technical limitation. it's not the path currently but we can split them if it makes more sense (i suspect as we have more integration tests, we will need to do this to run tests in parallel)

Besides parallel tests, there is also the case when you only want to run a specific test without downloading and starting the other containers that you don't need.
This could be a little bit complicated to set up, so we can ignore and handle in a follow up PR.

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

mauriciovasquezbernal · 2020-04-20T15:45:24Z

I forgot to say, an entry in the documentation is probably missing as well.

mauriciovasquezbernal

It's good to go. Just minor details on the comments, we can ignore the issue about putting all functional tests together and handle it later on.

I installed it, used docker-compose up in the integration tests folder and run the following script with the auto instrumentation command. I was able to see one span was printed to the console.

opentelemetry-auto-instrumentation redisexample.py

# configure otel related things
from opentelemetry import trace
from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.sdk.trace.export import (
    BatchExportSpanProcessor,
    ConsoleSpanExporter,
)

tracer_provider = TracerProvider()
trace.set_tracer_provider(tracer_provider)
exporter = ConsoleSpanExporter()
span_processor = BatchExportSpanProcessor(exporter)
tracer_provider.add_span_processor(span_processor)

# actual example starts here
import random
import redis
random.seed(444)
hats = {f"hat:{random.getrandbits(32)}": i for i in (
    {
        "color": "black",
        "price": 49.99,
        "style": "fitted",
        "quantity": 1000,
        "npurchased": 0,
    },
    {
        "color": "maroon",
        "price": 59.99,
        "style": "hipster",
        "quantity": 500,
        "npurchased": 0,
    },
    {
        "color": "green",
        "price": 99.99,
        "style": "baseball",
        "quantity": 200,
        "npurchased": 0,
    })
}

r = redis.Redis(db=1)

with r.pipeline() as pipe:
  for h_id, hat in hats.items():
    pipe.hmset(h_id, hat)
  pipe.execute()

ext/opentelemetry-ext-docker-tests/tests/redis/test_redis_functional.py

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

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

mauriciovasquezbernal · 2020-04-23T12:32:49Z

Regarding integration tests, shouldn't a check for redis be added to ext/opentelemetry-ext-docker-tests/tests/check_availability.py?

codeboten · 2020-04-23T15:20:40Z

Yes, it probably should. I didn't add it because in practice redis is basically available as soon as the container's up

mauriciovasquezbernal · 2020-04-24T13:37:58Z

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

+    trace.set_tracer_provider(TracerProvider())
+
+    # You can patch redis specifically
+    RedisInstrumentor().instrument(tracer_provider=trace.get_tracer_provider())


I think the general idea is to omit the tracer_provider parameter in the basic examples and let it as an advance use case. By default all the integrations should default to trace.get_tracer_provider() when the tracer provider is not passed.

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

ext/opentelemetry-ext-docker-tests/tests/check_availability.py

ext/opentelemetry-ext-docker-tests/tests/redis/test_redis_functional.py

ext/opentelemetry-instrumentation-redis/setup.cfg

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

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

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

c24t

I see this is the first package to use opentelemetry-instrumentation- for the package/distribution name. Is there a discussion for this somewhere? I see #283, but that's months out of date.

codeboten · 2020-04-24T20:37:13Z

I see there's a mention of this in the SIG notes from 04-09 https://docs.google.com/document/d/1CIMGoIOZ-c3-igzbd6_Pnxx1SjAkjwqoYSUWxPY8XIs/view#heading=h.ul61m6gnh8sm but nothing else. I can change it back if that's preferable.

c24t · 2020-04-24T20:48:55Z

I can change it back if that's preferable.

Only a slight preference as far as I'm concerned. I'd like to keep the names consistent and either rename all the instrumentation-extensions to -instrumentation or none of them.

Integration for PyMySQL. Fixes some documentation as well for other db integrations. Leverages dbapi. Co-authored-by: Mauricio Vásquez <[email protected]>

Co-Authored-By: Diego Hurtado <[email protected]>

codeboten · 2020-04-24T22:00:12Z

Only a slight preference as far as I'm concerned. I'd like to keep the names consistent and either rename all the instrumentation-extensions to -instrumentation or none of them.

I renamed it to ext

c24t

The tests look good, and it looks like a faithful port of https://github.com/open-telemetry/opentelemetry-python-contrib/blob/72b40ba5f94c320a1e6beb88971f0b1cb55541db/ddtrace/contrib/redis/patch.py.

c24t · 2020-04-24T22:00:03Z

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

+There are two options for instrumenting code. The first option is to use
+the `opentelemetry-auto-instrumentation` executable which will automatically


Suggested change

There are two options for instrumenting code. The first option is to use

the `opentelemetry-auto-instrumentation` executable which will automatically

There are two options for instrumenting code. The first option is to use the

``opentelemetry-auto-instrumentation`` executable which will automatically

To fix the sphinx target

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

c24t · 2020-04-24T22:37:36Z

ext/opentelemetry-ext-docker-tests/tests/redis/test_redis_functional.py

+        super().setUp()
+        self.redis_client = redis.Redis(port=6379)
+        self.redis_client.flushall()
+        RedisInstrumentor().instrument(tracer_provider=self.tracer_provider)


You want to call instrument/uninstrument once per test instead of once for the class?

Yeah, this way its not instrumenting the call to flushall which resets the redis data after each test.

…redis/__init__.py Co-Authored-By: Chris Kleinknecht <[email protected]>

ext/opentelemetry-ext-docker-tests/tests/check_availability.py

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

ocelotl

Approved! Just some minor nits/suggestions. 👍

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

ext/opentelemetry-ext-docker-tests/tests/redis/test_redis_functional.py

Co-Authored-By: Diego Hurtado <[email protected]>

toumorokoshi

Regarding the "instrumentation" convention, here's the PR for that:

open-telemetry/opentelemetry-specification#539

I believe it will be approved, just a matter of some work on my part to fix it up :)

Although if we're going to do this, I wonder if we just shouldn't rename them all at once.

toumorokoshi · 2020-04-26T04:42:56Z

ext/opentelemetry-ext-docker-tests/tests/redis/test_redis_functional.py

+        )
+
+        self.assertEqual(span.attributes.get("db.instance"), 0)
+        self.assertEqual(


is it worth refactoring some of these assertions into a utility method? there's a lot of identical code with regards to db.instance / url.

good call, refactored.

toumorokoshi · 2020-04-26T04:45:23Z

ext/opentelemetry-ext-redis/src/opentelemetry/ext/redis/util.py

+            ),
+            "db.instance": conn_kwargs["db"] or 0,
+        }
+    except KeyError:


is it worth logging this condition? I guess this is just a quick way of writing that if any params or missing, don't include any of these.

not sure about logging there, i haven't looked into what conditions would cause those keys to be missing

toumorokoshi · 2020-04-26T04:45:31Z

ext/opentelemetry-ext-redis/src/opentelemetry/ext/redis/util.py

+    """ Transform redis conn info into dict """
+    try:
+        return {
+            "db.type": "redis",


should this be added regardless?

yes, updated the code to set available attributes

Exporter and span handling complexity was increasing due to the Span.parent attribute being either a Span or a SpanContext. As there is no requirement in the specification to have the parent attribute be a Span, removing this complexity and handling. Co-authored-by: Chris Kleinknecht <[email protected]> Co-authored-by: Alex Boten <[email protected]>

* feat(xml-http-request): new plugin for auto instrumentation for xml-http-request * chore: new example for xml-http-request and updated examples structure for web * chore: updating readme * chore: linting * chore: fixing origin for tests * chore: linting * chore: updating to use b3 format from core * chore: updates after reviews * chore: wrong function call * chore: updating attribute names * chore: linting * chore: adding preflight requests, fixing few other issues * chore: adding image to examples, updating readme * chore: forcing async to be true, but propagate rest params * chore: fixing type for open and send function * chore: fixing format for headers * chore: reviews * chore: decrement task count when span exists * chore: changes after review * chore: adding weakmap for keeping information connected with xhr * chore: renaming config param * chore: not needed cast * chore: updating title * chore: refactored xhr, removed tracing dependency, few other issues fixed * chore: reviews * chore: refactored for collecting timing resources * chore: fixes after merging * chore: reviews * chore: reviews Co-authored-by: Mayur Kale <[email protected]>

porting redis instrumentation

f91ce7c

codeboten requested a review from a team April 17, 2020 16:34

codeboten mentioned this pull request Apr 17, 2020

Instrumenting Redis open-telemetry/opentelemetry-python-contrib#21

Closed

fix lint and docker-tests

88ea6f1

codeboten added the instrumentation Related to the instrumentation of third party libraries or frameworks label Apr 17, 2020

mauriciovasquezbernal reviewed Apr 20, 2020

View reviewed changes

codeboten added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Apr 20, 2020

Alex Boten added 8 commits April 20, 2020 13:47

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

8799497

small cleanup

4157cbf

improving documentation

1e48c7d

expect a tracer_provider instead of a tracer

d0af1ec

fixing tox.ini

6809d9a

fixing tox file

64e1d72

removing reload, it breaks checks for Span types

2e71d4f

remove reload

a98fb47

mauriciovasquezbernal approved these changes Apr 22, 2020

View reviewed changes

Alex Boten added 5 commits April 22, 2020 08:03

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

43ae8b0

cleaning up tests

8933c0b

adding comment to remember to update how the tracer is set

9b7e311

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

5e8172d

updating code to use instrument/uninstrument interface

a33287b

Alex Boten added 5 commits April 23, 2020 08:41

adding check for redis, refactoring and improving error handling

5f40067

clean unused import

2bcadf5

adding changelog

65c6cb3

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

3a1d1dd

update doc

5fd98fe

mauriciovasquezbernal reviewed Apr 24, 2020

View reviewed changes

ocelotl suggested changes Apr 24, 2020

View reviewed changes

c24t reviewed Apr 24, 2020

View reviewed changes

lzchen and others added 8 commits April 24, 2020 14:23

PyMySQL Integration (open-telemetry#504)

039ad13

Integration for PyMySQL. Fixes some documentation as well for other db integrations. Leverages dbapi. Co-authored-by: Mauricio Vásquez <[email protected]>

review feedback

75a9bf4

review feedback

28e6d7c

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

9913c1c

Apply suggestions from code review

1b978a5

Co-Authored-By: Diego Hurtado <[email protected]>

adding version for redis

5d74317

more review feedback changes

d843cad

rename from instrumentation to ext

e064dc1

c24t approved these changes Apr 24, 2020

View reviewed changes

Update ext/opentelemetry-ext-redis/src/opentelemetry/instrumentation/…

2712732

…redis/__init__.py Co-Authored-By: Chris Kleinknecht <[email protected]>

ocelotl suggested changes Apr 24, 2020

View reviewed changes

ext/opentelemetry-ext-docker-tests/tests/check_availability.py Outdated Show resolved Hide resolved

ext/opentelemetry-ext-redis/src/opentelemetry/instrumentation/redis/__init__.py Outdated Show resolved Hide resolved

Alex Boten added 2 commits April 24, 2020 16:05

fixing exception in check_availability

8907032

finish renaming, remove patch.py

5ee71de

ocelotl approved these changes Apr 24, 2020

View reviewed changes

Alex Boten and others added 2 commits April 24, 2020 16:48

fix lint

2faca48

Apply suggestions from code review

f372d1e

Co-Authored-By: Diego Hurtado <[email protected]>

toumorokoshi approved these changes Apr 26, 2020

View reviewed changes

toumorokoshi and others added 5 commits April 26, 2020 14:48

fix lint

a10916b

refactor tests

0757069

set attributes that are available

e4d9084

Merge branch 'master' into instrumentation-redis

b4d810e

toumorokoshi merged commit a832bb0 into open-telemetry:master Apr 27, 2020

mauriciovasquezbernal mentioned this pull request Apr 29, 2020

Updating redis instrumentation to use instrumentor interface open-telemetry/opentelemetry-python-contrib#38

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Porting redis instrumentation from contrib repo #595

Porting redis instrumentation from contrib repo #595

codeboten commented Apr 17, 2020

mauriciovasquezbernal left a comment •

edited

Loading

mauriciovasquezbernal Apr 20, 2020

codeboten Apr 20, 2020

mauriciovasquezbernal Apr 22, 2020

mauriciovasquezbernal commented Apr 20, 2020

mauriciovasquezbernal left a comment

mauriciovasquezbernal commented Apr 23, 2020

codeboten commented Apr 23, 2020

mauriciovasquezbernal Apr 24, 2020

c24t left a comment

codeboten commented Apr 24, 2020

c24t commented Apr 24, 2020 •

edited

Loading

codeboten commented Apr 24, 2020

c24t left a comment

c24t Apr 24, 2020

c24t Apr 24, 2020

codeboten Apr 24, 2020

ocelotl left a comment

toumorokoshi left a comment

toumorokoshi Apr 26, 2020

codeboten Apr 26, 2020

toumorokoshi Apr 26, 2020

codeboten Apr 26, 2020

toumorokoshi Apr 26, 2020

codeboten Apr 26, 2020

		There are two options for instrumenting code. The first option is to use
		the `opentelemetry-auto-instrumentation` executable which will automatically

Porting redis instrumentation from contrib repo #595

Porting redis instrumentation from contrib repo #595

Conversation

codeboten commented Apr 17, 2020

mauriciovasquezbernal left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented Apr 20, 2020

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented Apr 23, 2020

codeboten commented Apr 23, 2020

Choose a reason for hiding this comment

c24t left a comment

Choose a reason for hiding this comment

codeboten commented Apr 24, 2020

c24t commented Apr 24, 2020 • edited Loading

codeboten commented Apr 24, 2020

c24t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocelotl left a comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment •

edited

Loading

c24t commented Apr 24, 2020 •

edited

Loading