Add pymemcache instrumentation by ericmustin · Pull Request #772 · open-telemetry/opentelemetry-python

ericmustin · 2020-06-03T15:13:52Z

summary

👋 This PR adds pymemcache instrumentation and tests, addressing: #766 . Still a few open questions I have, but at a place where I think this can be reviewed. By and large I tried to port things from dd-trace-py's pymemcache instumentation

Open Questions / Todos:
- not 100% how to handle changelog and versioning, as well as setup.cfg and setup.py files. Took some educated guesses and basically just borrowed the structure of other integrations as a guide, let me know if I've botched anything here
- Tried to attach licenses everywhere I could but lmk if I've missed anything.
- this opentelemetry-ext-pymemcache implementation differs a bit from dd-trace-py, in that dd-trace-py replaces the entire Client with a wrapped client, whereas opentelemetry-ext-pymemcache is only wrapping specific methods on the original Client. The dd-trace-py approach relies on Pin, which isn't a construct available in opentelemetry-python afaik. There were a few gotchas with this approach (like ensuring methods aren't patched multiple times) but overall it behaves the same.
- Tried to write the tests as closely as I could to the originals in dd-trace-py but was not super sure how to test some of config/client setting stuff, so left that out for now.
Example Usage:

    from opentelemetry import trace
    from opentelemetry.sdk.trace import TracerProvider
    from opentelemetry.ext.pymemcache import PymemcacheInstrumentor
    trace.set_tracer_provider(TracerProvider())
    from pymemcache.client.base import Client
    client = Client(('localhost', 11211))
    client.set('some_key', 'some_value')

Thanks for taking a look, happy to help however i can and address any feedback.

…uble wrapping

majorgreys

LGTM on a first pass!

majorgreys · 2020-06-04T13:36:39Z

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

+"""
+Usage
+-----
+The OpenTelemetry ``jinja2`` integration traces templates loading, compilation


Update this usage

👍 good catch , updated

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

majorgreys · 2020-06-04T13:48:41Z

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

+        _CMD, kind=SpanKind.INTERNAL, attributes={}
+    ) as span:
+        try:
+            span.set_attribute("service", tracer.instrumentation_info.name)


The service name is handled by the Resource in OpenTelemetry so we can skip setting this attribute.

ah gotcha, makes sense, i've removed this entirely then.

… example usage

cnnradams

LGTM! Have a few minor comments, but other than that looks great!

cnnradams · 2020-06-05T17:13:26Z

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

+    from opentelemetry.ext.pymemcache import PymemcacheInstrumentor
+    trace.set_tracer_provider(TracerProvider())
+    from pymemcache.client.base import Client
+    client = Client(('localhost', 11211))


Should there be a PymemcacheInstrumentor().instrument() before creating the client?

ah good catch, ty, updated

cnnradams · 2020-06-05T17:20:37Z

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

+    See `BaseInstrumentor`
+    """
+
+    def _instrument(self, **kwargs):


A lot of other similar instrumentors also expose an instrument_client(client) function that would instrument methods on a single instantiated Client instead of every Client in every file that is created. Is that possible here, and if so do you think it provides enough value to add?

I think that sounds possible, I think some of the dd-trace-py instrumentations expose something to that effect, and it could have some value, but I don't think it's trivial to add. Is that convention established anywhere in opentelemetry-python at the moment?

good question, I don't think it's written down anywhere, I just see it a lot (all db instrumentations,most webserver instrumentations). There's probably no need to add it in this PR unless there was actually a use case where you would want to instrument one client but not others

no strong convention there, but see flask for an example:https://github.com/open-telemetry/opentelemetry-python/blob/master/ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py#L194

One thing is that "app" is probably a misnomer in the context of memcache, so the convention is loose at best.

There is no convention for any other method outside _instrument and _uninstrument. Since these instrumentations deal with different third party libraries it is ok to add more methods as needed (like it was done for the Flask instrumentation) to better adapt the instrumentation to the characteristics of the third party libraries.

cnnradams · 2020-06-05T20:04:33Z

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

+                    instance.server
+                )
+
+    except Exception:  # pylint: disable=broad-except


What would cause an exception here?

just was being overly defensive, removed

cnnradams · 2020-06-05T20:09:07Z

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

+    elif isinstance(arg, bytes):
+        keys = arg.decode()
+    elif isinstance(arg, list) and len(arg) >= 1:
+        if isinstance(arg[0], str):


are we guaranteed that every argument in the argument list is a string if the first one is? not familiar with pymemcache, so maybe it is guaranteed

Not a pymemcache expert but yes I believe that's the case, first arg is either:

keys, a list or dict of key strings, like here: https://github.com/pinterest/pymemcache/blob/f02ddf73a28c09256589b8afbb3ee50f1171cac7/pymemcache/client/base.py#L503

or a single key, a string: https://github.com/pinterest/pymemcache/blob/f02ddf73a28c09256589b8afbb3ee50f1171cac7/pymemcache/client/base.py#L324

Also, just to clarify, we're only grabbing the keys of the query, not the full command

owais · 2020-06-04T16:54:54Z

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

+
+            _set_connection_attributes(span, instance)
+        except Exception:  # pylint: disable=broad-except
+            pass


Should this be logged?

👍 added a warning log

owais · 2020-06-05T19:49:41Z

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

+    return address_attributes
+
+
+def _get_query_string(args):


If this function only ever deals with the first element of the list, may be it should take only a single element as argument instead of a list?

Also, this function is only used in one place in your code, better to move it there to avoid having to jump to some other file to read your code.

@owais agreed, i've updated this function (and moved it to the same file as the function where it's being called) to simplify things a bit

ocelotl · 2020-06-08T15:16:27Z

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

+    # pull out the first arg which will contain any key
+    arg = args[0]
+
+    # if we get a dict, convert to list of keys


Nit: these comments are a bit unnecessary

fair enough, removed

ocelotl · 2020-06-08T15:41:07Z

ext/opentelemetry-ext-pymemcache/tests/test_pymemcache.py

+    def test_set_success(self):
+        client = self.make_client([b"STORED\r\n"])
+        result = client.set(b"key", b"value", noreply=False)
+        assert result is True


Suggested change

assert result is True

self.assertTrue(result)

👍 updated

ocelotl

Just some minor comments

linux-foundation-easycla · 2020-06-09T08:15:18Z

The committers are authorized under a signed CLA.

✅ Eric Mustin (4c40a9a, c1ce4bc, 0853b7d, a19e11c, fb0e7a4, 430d5d4, e066b96, bd3b802, 8f70c5a, 09fb322, d765e3e, e20d38c, 1e31316, 25f19d7, 5f45063, 9705321, ea7e444, b283828, 8dd762e, 0332711, 29ababc, 9008f73, de89616, 6e21abf, f3a1592, 24e6077, a527365)

merge master

toumorokoshi

not 100% how to handle changelog and versioning, as well as setup.cfg and setup.py files. Took some educated guesses and basically just borrowed the structure of other integrations as a guide, let me know if I've botched anything here

Looks good! Not much to it. See https://github.com/open-telemetry/opentelemetry-python/blob/master/RELEASING.md for our whole process which includes bumping versions.

this opentelemetry-ext-pymemcache implementation differs a bit from dd-trace-py, in that dd-trace-py replaces the entire Client with a wrapped client, whereas opentelemetry-ext-pymemcache is only wrapping specific methods on the original Client. The dd-trace-py approach relies on Pin, which isn't a construct available in opentelemetry-python afaik. There were a few gotchas with this approach (like ensuring methods aren't patched multiple times) but overall it behaves the same.

Do you have any docs / links to "Pin"? Maybe it's a concept we should add in.

Tried to write the tests as closely as I could to the originals in dd-trace-py but was not super sure how to test some of config/client setting stuff, so left that out for now.

Do you have links? curious about these as well.

Thanks! overall LGTM.

toumorokoshi · 2020-06-09T17:35:19Z

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

+    return keys
+
+
+def _unwrap(obj, attr):


this is a now a utility in opentelemetry-instrumentation. it would be great to refactor to use that method.

ah nice, updated to use the util method

toumorokoshi · 2020-06-09T17:36:27Z

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

+    See `BaseInstrumentor`
+    """
+
+    def _instrument(self, **kwargs):


no strong convention there, but see flask for an example:https://github.com/open-telemetry/opentelemetry-python/blob/master/ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py#L194

One thing is that "app" is probably a misnomer in the context of memcache, so the convention is loose at best.

ocelotl

Approving, I commented about a couple minor changes that could be addressed 👍

ocelotl · 2020-06-09T17:46:35Z

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

+from opentelemetry.ext.pymemcache.version import __version__
+from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
+from opentelemetry.trace import SpanKind, get_tracer
+from opentelemetry.trace.status import Status, StatusCanonicalCode


These two are unused

good catch, updated (removed)

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

ocelotl · 2020-06-09T17:50:11Z

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

+    See `BaseInstrumentor`
+    """
+
+    def _instrument(self, **kwargs):


There is no convention for any other method outside _instrument and _uninstrument. Since these instrumentations deal with different third party libraries it is ok to add more methods as needed (like it was done for the Flask instrumentation) to better adapt the instrumentation to the characteristics of the third party libraries.

toumorokoshi · 2020-06-09T17:52:52Z

@ericmustin do you have time to look at Diego's comments today? We're good to merge after maybe a final look by you.

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

lzchen · 2020-06-09T18:09:47Z

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py

+        return wrapped(*args, **kwargs)
+
+
+def _get_query_string(arg):


I'm wondering if some of these methods defined here could go into util.py and leave only the wrapping, instrumenting code in __init__.py.

@ericmustin Thoughts on this?

apologies, i missed this comment. yea i mean, i don't have strong opinions here at all, this was actually originally in util and it was suggested to move it here 😅 #772 (comment)

Happy to move _get_query_string back if that's preferred

@ocelotl has a point that it's only being used once. Maybe just move all the funcitons into this file and get rid of the utils.py?

Yup, I think that probably makes sense, updated (merged util.py helpers into __init__.py and removed util.py ).

lzchen

LGTM. Some non-blocking comments.

ericmustin · 2020-06-09T20:32:45Z

@toumorokoshi apologies, i'm afk rn (i'm in CEST timezone 🇫🇷 ) so likely cannot grab these before EOD US 🇺🇸 . let me circle back to these first thing in the morning, everyone's comments seem very reasonable i should be able to address them pretty quickly. thanks for all the the thoughtful reviews everyone!

toumorokoshi · 2020-06-09T20:36:14Z

@toumorokoshi apologies, i'm afk rn (i'm in CEST timezone ) so likely cannot grab these before EOD US . let me circle back to these first thing in the morning, everyone's comments seem very reasonable i should be able to address them pretty quickly. thanks for all the the thoughtful reviews everyone!

no worries! sorry there's no timeline here. Just @ me when things look good and I'll merge.

ericmustin · 2020-06-10T13:03:53Z

@toumorokoshi I think I've addressed the relevant feedback here from @ocelotl , @lzchen , @cnnradams and others. Thanks again all for the great feedback! I'm still a little bit confused on what I should include in the CHANGELOG for pymemcache, and whether I should be cutting a release or whatnot, I assume not since I'm not a maintainer?

Addressing the other feedback points:

regarding the question on Pin
- Do you have any docs / links to "Pin"? Maybe it's a concept we should add in.

Yup sure, you can find some details on it in the dd-trace-py pypi docs
- As I understand it, it basically allows user to add specific context to different connections, and it exposes to helper methods to do so
- Pin (a.k.a Patch Info) is a small class which is used to set tracing metadata on a particular traced connection. This is useful if you wanted to, say, trace two different database clusters.

regarding the client/config tests that i didn't port over

for reference they can be found here . Taking a closer look it's mostly just testing some datadog specific settings, so I don't believe it's useful.

Lastly, regarding adding a method to trace specific pymemcache clients, as opposed to all of them.

I think it's useful and the flask example is helpful to look at for guidance, but would prefer to move it outside the scope of this PR, as I think it's probably worth considering adding a general helper class like "Pin" first to make this sort of work more standard.

Lmk if there's anything else needed here or I've missed something, happy to help make sure this gets brought across the finish line.

lzchen · 2020-06-10T17:04:43Z

@ericmustin
As for your CHANGELOG question, a good example would be this PR in what to include for the CHANGELOG for a new package. Place it under unreleased and when we do an actual release, the maintainers will update all the CHANGELOG and version.py files to the correct ones.

toumorokoshi · 2020-06-11T22:57:35Z

@ericmustin sorry, but this needs to be updated to the new version (0.10.dev.0) since that was released yesterday.

can you either update the branch yourself, or change the PR configuration so we can make changes to your branch?

merge master

ericmustin · 2020-06-12T08:06:20Z

@toumorokoshi Updated to 0.10.dev.0 , thanks! Going forward i'll allow maintainer edits on the PR to make stuff like this easier.

toumorokoshi · 2020-06-12T14:45:49Z

Great, thanks a lot! Sorry for all the back and forth.

ericmustin added 12 commits June 2, 2020 02:09

ext/pymemcache: wip instrumentation, config dev env setup

4c40a9a

ext/pymemcache: fix util functions for query and connection attributes

c1ce4bc

ext/pymemcache: add license info and cleanup formatting

0853b7d

Merge branch 'master' into add_pymemcache

a19e11c

ext/pymemcache: linting and add pymemcache span type to datadog exporter

fb0e7a4

ext/pymemcache: update url syntax

430d5d4

ext/pymemcache: add base spec and first test'

e066b96

ext/pymemcache: linting

bd3b802

ext/pymemcache: add tests for base behaviors and commands, prevent do…

8f70c5a

…uble wrapping

ext/pymemcache: add error and uninstrumentation tests, fix unwrapping

09fb322

ext/pymemcache: linting of tests

d765e3e

ext/pymemcache: test pooled client

e20d38c

ericmustin requested a review from a team June 3, 2020 15:13

ericmustin added 6 commits June 3, 2020 18:02

ext/pymemcache: fix docs linting

1e31316

ext/pymemcache: linting and doc fix

25f19d7

ext/pymemcache: more linting fixing pylint

5f45063

ext/pymemcache: reformat from black

9705321

ext/pymemcache: remove pytest in unit tests

ea7e444

ext/pymemcache: run black

b283828

majorgreys suggested changes Jun 4, 2020

View reviewed changes

ericmustin added 3 commits June 4, 2020 16:36

ext/pymemcache: readme linting

8dd762e

ext/pymemcache: remove unncessary service attribute setting, clean up…

0332711

… example usage

ext/pymemcache: doc requirements fix

29ababc

cnnradams approved these changes Jun 5, 2020

View reviewed changes

owais reviewed Jun 6, 2020

View reviewed changes

ocelotl reviewed Jun 8, 2020

View reviewed changes

ext/pymemcache: implement feedback simplify code a bit add logs

9008f73

ericmustin added 5 commits June 9, 2020 10:22

Merge branch 'master' into add_pymemcache

de89616

merge master

ext/pymemcache: bring up to date with otel master

6e21abf

ext/pymemcache: update to point to new autoinstrumentation package

f3a1592

ext/pymemcache: update BaseInstrumentation path

24e6077

ext/pymemcache: linting pymemcache

a527365

toumorokoshi approved these changes Jun 9, 2020

View reviewed changes

ocelotl approved these changes Jun 9, 2020

View reviewed changes

lzchen reviewed Jun 9, 2020

View reviewed changes

ext/opentelemetry-ext-pymemcache/src/opentelemetry/ext/pymemcache/__init__.py Show resolved Hide resolved

lzchen reviewed Jun 9, 2020

View reviewed changes

lzchen approved these changes Jun 9, 2020

View reviewed changes

ericmustin added 2 commits June 10, 2020 14:27

ext/pymemcache: remove unused imports

0ef71f5

use unwrap util

7069d48

ext/pymemcache: add changelog

bd07995

ericmustin requested a review from majorgreys June 10, 2020 19:34

ext/pymemcache: remove util file just include it all in init

8f940ec

majorgreys approved these changes Jun 11, 2020

View reviewed changes

ericmustin added 2 commits June 12, 2020 09:57

Merge branch 'master' into add_pymemcache

99cfbb5

merge master

update to 0.10 version

c07ed5e

toumorokoshi merged commit ca232c9 into open-telemetry:master Jun 12, 2020

ericmustin mentioned this pull request Jul 16, 2020

Add instrumentation for pymemcache #766

Closed

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

chore: add typing to propagator carrier (open-telemetry#772)

4faac48

Conversation

ericmustin commented Jun 3, 2020

summary

Uh oh!

majorgreys left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnnradams left a comment

Choose a reason for hiding this comment

Uh oh!

cnnradams Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cnnradams Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cnnradams Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ocelotl Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

linux-foundation-easycla bot commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toumorokoshi left a comment

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!

cnnradams Jun 5, 2020 •

edited

Loading

cnnradams Jun 9, 2020 •

edited

Loading

cnnradams Jun 5, 2020 •

edited

Loading

ocelotl Jun 8, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jun 9, 2020 •

edited

Loading