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

Update pymemcache instrumentation to support pymemcache >=1.3.5,<4 #935

Merged
merged 1 commit into from
Apr 6, 2022
Merged

Update pymemcache instrumentation to support pymemcache >=1.3.5,<4 #935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `opentelemetry-instrumentation-kafka-python` Fix topic extraction
([#949](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/949))

### Changed

- `opentelemetry-instrumentation-pymemcache` should run against newer versions of pymemcache.
([#935](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/935))

## [1.9.1-0.28b1](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.9.1-0.28b1) - 2022-01-29

### Fixed
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
| [opentelemetry-instrumentation-mysql](./opentelemetry-instrumentation-mysql) | mysql-connector-python ~= 8.0 |
| [opentelemetry-instrumentation-pika](./opentelemetry-instrumentation-pika) | pika >= 0.12.0 |
| [opentelemetry-instrumentation-psycopg2](./opentelemetry-instrumentation-psycopg2) | psycopg2 >= 2.7.3.1 |
| [opentelemetry-instrumentation-pymemcache](./opentelemetry-instrumentation-pymemcache) | pymemcache ~= 1.3 |
| [opentelemetry-instrumentation-pymemcache](./opentelemetry-instrumentation-pymemcache) | pymemcache >= 1.3.5, < 4 |
| [opentelemetry-instrumentation-pymongo](./opentelemetry-instrumentation-pymongo) | pymongo >= 3.1, < 5.0 |
| [opentelemetry-instrumentation-pymysql](./opentelemetry-instrumentation-pymysql) | PyMySQL < 2 |
| [opentelemetry-instrumentation-pyramid](./opentelemetry-instrumentation-pyramid) | pyramid >= 1.7 |
Expand Down
2 changes: 1 addition & 1 deletion ...emetry-instrumentation-pymemcache/src/opentelemetry/instrumentation/pymemcache/package.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@
# limitations under the License.


_instruments = ("pymemcache ~= 1.3",)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After testing, there was a change in 1.3.5 that added version to the client. Before this the instrumentation fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

So for versions < 1.3.5, the instrumentation did not work properly? According to this, a VERSION command was added. Was curious as to why without this it would cause the instrumentation to break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so the instrumentation uses .version() in the tests. This leads to a test failing. While I am fairly confident it would work for versions < 1.3.5, the tests don't and I wanted to keep this PRs scope low given all of the issues I am already currently having.

So locking the version to what actually passes the test felt like a good start.

_instruments = ("pymemcache >= 1.3.5, < 4",)
66 changes: 53 additions & 13 deletions instrumentation/opentelemetry-instrumentation-pymemcache/tests/test_pymemcache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
from unittest import mock

import pymemcache
from packaging import version as get_package_version
from pymemcache import __version__ as pymemcache_package_version
from pymemcache.exceptions import (
MemcacheClientError,
MemcacheIllegalInputError,
Expand All @@ -33,6 +35,22 @@
TEST_HOST = "localhost"
TEST_PORT = 117711

pymemcache_version = get_package_version.parse(pymemcache_package_version)

# In pymemcache versions greater than 2, set_multi, set_many and delete_multi
# now use a batched call
# https://github.com/pinterest/pymemcache/blob/master/ChangeLog.rst#new-in-version-200 # noqa
pymemcache_version_lt_2 = pymemcache_version < get_package_version.parse(
rjduffner marked this conversation as resolved.
Show resolved Hide resolved
"2.0.0"
)

# In pymemcache versions greater than 3.4.1, the stats command no
# longer sends trailing whitespace in the command
# https://github.com/pinterest/pymemcache/blob/master/ChangeLog.rst#new-in-version-342 # noqa
pymemcache_version_gt_341 = pymemcache_version > get_package_version.parse(
lzchen marked this conversation as resolved.
Show resolved Hide resolved
"3.4.1"
)


class PymemcacheClientTestCase(
TestBase
Expand Down Expand Up @@ -126,11 +144,16 @@ def test_set_multi_success(self):
client = self.make_client([b"STORED\r\n"])
# Alias for set_many, a convienance function that calls set for every key
result = client.set_multi({b"key": b"value"}, noreply=False)
self.assertTrue(result)

spans = self.memory_exporter.get_finished_spans()

self.check_spans(spans, 2, ["set key", "set_multi key"])
# In pymemcache versions greater than 2, set_multi now uses a batched call
# https://github.com/pinterest/pymemcache/blob/master/ChangeLog.rst#new-in-version-200 # noqa
if pymemcache_version_lt_2:
self.assertTrue(result)
self.check_spans(spans, 2, ["set key", "set_multi key"])
else:
assert len(result) == 0
self.check_spans(spans, 1, ["set_multi key"])

def test_delete_not_found(self):
client = self.make_client([b"NOT_FOUND\r\n"])
Expand Down Expand Up @@ -191,19 +214,30 @@ def test_delete_many_found(self):

spans = self.memory_exporter.get_finished_spans()

self.check_spans(
spans, 3, ["add key", "delete key", "delete_many key"]
)
# In pymemcache versions greater than 2, delete_many now uses a batched call
# https://github.com/pinterest/pymemcache/blob/master/ChangeLog.rst#new-in-version-200 # noqa
if pymemcache_version_lt_2:
self.check_spans(
spans, 3, ["add key", "delete key", "delete_many key"]
)
else:
self.check_spans(spans, 2, ["add key", "delete_many key"])

def test_set_many_success(self):
client = self.make_client([b"STORED\r\n"])
# a convienance function that calls set for every key
result = client.set_many({b"key": b"value"}, noreply=False)
self.assertTrue(result)

spans = self.memory_exporter.get_finished_spans()

self.check_spans(spans, 2, ["set key", "set_many key"])
# In pymemcache versions greater than 2, set_many now uses a batched call
# https://github.com/pinterest/pymemcache/blob/master/ChangeLog.rst#new-in-version-200 # noqa
if pymemcache_version_lt_2:
self.assertTrue(result)
self.check_spans(spans, 2, ["set key", "set_many key"])
else:
assert len(result) == 0
self.check_spans(spans, 1, ["set_many key"])

def test_set_get(self):
client = self.make_client(
Expand Down Expand Up @@ -243,7 +277,7 @@ def test_prepend_stored(self):

def test_cas_stored(self):
client = self.make_client([b"STORED\r\n"])
result = client.cas(b"key", b"value", b"cas", noreply=False)
result = client.cas(b"key", b"value", b"1", noreply=False)
self.assertTrue(result)

spans = self.memory_exporter.get_finished_spans()
Expand All @@ -252,7 +286,7 @@ def test_cas_stored(self):

def test_cas_exists(self):
client = self.make_client([b"EXISTS\r\n"])
result = client.cas(b"key", b"value", b"cas", noreply=False)
result = client.cas(b"key", b"value", b"1", noreply=False)
assert result is False

spans = self.memory_exporter.get_finished_spans()
Expand All @@ -261,7 +295,7 @@ def test_cas_exists(self):

def test_cas_not_found(self):
client = self.make_client([b"NOT_FOUND\r\n"])
result = client.cas(b"key", b"value", b"cas", noreply=False)
result = client.cas(b"key", b"value", b"1", noreply=False)
assert result is None

spans = self.memory_exporter.get_finished_spans()
Expand Down Expand Up @@ -445,7 +479,14 @@ def test_version_success(self):
def test_stats(self):
client = self.make_client([b"STAT fake_stats 1\r\n", b"END\r\n"])
result = client.stats()
assert client.sock.send_bufs == [b"stats \r\n"]

# In pymemcache versions greater than 3.4.1, the stats command no
# longer sends trailing whitespace in the command
# https://github.com/pinterest/pymemcache/blob/master/ChangeLog.rst#new-in-version-342 # noqa
if pymemcache_version_gt_341:
assert client.sock.send_bufs == [b"stats\r\n"]
else:
assert client.sock.send_bufs == [b"stats \r\n"]
assert result == {b"fake_stats": 1}

spans = self.memory_exporter.get_finished_spans()
Expand Down Expand Up @@ -544,5 +585,4 @@ def test_delete_many_found(self):
self.assertTrue(result)

spans = self.memory_exporter.get_finished_spans()

self.check_spans(spans, 2, ["add key", "delete key"])
2 changes: 1 addition & 1 deletion opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@
"instrumentation": "opentelemetry-instrumentation-psycopg2==0.29b0",
},
"pymemcache": {
"library": "pymemcache ~= 1.3",
"library": "pymemcache >= 1.3.5, < 4",
"instrumentation": "opentelemetry-instrumentation-pymemcache==0.29b0",
},
"pymongo": {
Expand Down
12 changes: 8 additions & 4 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,8 @@ envlist =
; ext-psycopg2 intentionally excluded from pypy3

; opentelemetry-instrumentation-pymemcache
py3{6,7,8,9,10}-test-instrumentation-pymemcache
pypy3-test-instrumentation-pymemcache
py3{6,7,8,9,10}-test-instrumentation-pymemcache{135,200,300,342}
pypy3-test-instrumentation-pymemcache{135,200,300,342}

; opentelemetry-instrumentation-pymongo
py3{6,7,8,9,10}-test-instrumentation-pymongo
Expand Down Expand Up @@ -222,6 +222,10 @@ deps =
sqlalchemy14: sqlalchemy~=1.4
pika0: pika>=0.12.0,<1.0.0
pika1: pika>=1.0.0
pymemcache135: pymemcache ==1.3.5
pymemcache200: pymemcache >2.0.0,<3.0.0
pymemcache300: pymemcache >3.0.0,<3.4.2
pymemcache342: pymemcache ==3.4.2
httpx18: httpx>=0.18.0,<0.19.0
httpx18: respx~=0.17.0
httpx21: httpx>=0.19.0
Expand Down Expand Up @@ -262,7 +266,7 @@ changedir =
test-instrumentation-mysql: instrumentation/opentelemetry-instrumentation-mysql/tests
test-instrumentation-pika{0,1}: instrumentation/opentelemetry-instrumentation-pika/tests
test-instrumentation-psycopg2: instrumentation/opentelemetry-instrumentation-psycopg2/tests
test-instrumentation-pymemcache: instrumentation/opentelemetry-instrumentation-pymemcache/tests
test-instrumentation-pymemcache{135,200,300,342}: instrumentation/opentelemetry-instrumentation-pymemcache/tests
test-instrumentation-pymongo: instrumentation/opentelemetry-instrumentation-pymongo/tests
test-instrumentation-pymysql: instrumentation/opentelemetry-instrumentation-pymysql/tests
test-instrumentation-pyramid: instrumentation/opentelemetry-instrumentation-pyramid/tests
Expand Down Expand Up @@ -332,7 +336,7 @@ commands_pre =

mysql: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-dbapi {toxinidir}/instrumentation/opentelemetry-instrumentation-mysql[test]

pymemcache: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-pymemcache[test]
pymemcache{135,200,300,342}: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-pymemcache[test]

pymongo: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-pymongo[test]

Expand Down