Skip to content

Commit 3976ec2

Browse files
github-actions[bot]wantsuierikayasuda
authored
fix(pylibmc): support client initialization using servers [backport 3.2] (#12785)
Backport e08d99c from #12721 to 3.2. Currently calling `Client(servers=[url])` throws an error as seen with the new test: [Example CI error](https://gitlab.ddbuild.io/DataDog/apm-reliability/dd-trace-py/-/jobs/847335230) ``` FAILED tests/contrib/pylibmc/test.py::TestPylibmcPatch::test_client_with_servers_option - TypeError: __init__() got multiple values for argument 'servers' FAILED tests/contrib/pylibmc/test.py::TestPylibmcPatch::test_client_with_servers_option - TypeError: __init__() got multiple values for argument 'servers' FAILED tests/contrib/pylibmc/test.py::TestPylibmcPatch::test_client_with_servers_option - TypeError: __init__() got multiple values for argument 'servers' ``` This is a bug in our integration logic because you can declare Clients with and without the `servers` portion: https://sendapatch.se/projects/pylibmc/reference.html . **Why did this bug occur?** When you call `pylibmc.Client(servers=[url])`, by the time it hits our patch code, `**kwargs` already stores `servers`. Then when this gets passed into the Client init, it sees two "servers" https://github.com/lericson/pylibmc/blob/1183e48eb86adf0c23c5164bfe2c2a4058234640/src/pylibmc/client.py#L126, hence the complaint about the multiple server arguments. A quick fix is to check if the kwargs contains servers and set up the client without duplicating it. Fixes APMS-15146 ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) Co-authored-by: wantsui <[email protected]> Co-authored-by: erikayasuda <[email protected]>
1 parent 15f5a7f commit 3976ec2

File tree

3 files changed

+35
-1
lines changed

3 files changed

+35
-1
lines changed

ddtrace/contrib/internal/pylibmc/client.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ def __init__(self, client=None, service=memcached.SERVICE, tracer=None, *args, *
4141
if not isinstance(client, _Client):
4242
# We are in the patched situation, just pass down all arguments to the pylibmc.Client
4343
# Note that, in that case, client isn't a real client (just the first argument)
44-
client = _Client(client, *args, **kwargs)
44+
client = _Client(kwargs.get("servers", client), **{k: v for k, v in kwargs.items() if k != "servers"})
4545
else:
4646
log.warning(
4747
"TracedClient instantiation is deprecated and will be remove "
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
pylibmc: fixes an issue where using ``Client(server=[url])`` would throw the error ``__init__() got multiple values for argument 'servers'``

tests/contrib/pylibmc/test.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,6 +371,36 @@ def test_patch_unpatch(self):
371371
assert spans, spans
372372
assert len(spans) == 1
373373

374+
def test_client_with_servers_option(self):
375+
"""
376+
A test to make sure we support using `servers`, ie: with pylibmc.Client(servers=[url])
377+
"""
378+
url = "%s:%s" % (cfg["host"], cfg["port"])
379+
380+
patch()
381+
client = pylibmc.Client(servers=[url])
382+
assert client.addresses[0] is url
383+
Pin.get_from(client)._clone(service=self.TEST_SERVICE, tracer=self.tracer).onto(client)
384+
client.set("a", 1)
385+
spans = self.pop_spans()
386+
assert spans, spans
387+
assert len(spans) == 1
388+
389+
def test_client_without_servers_option(self):
390+
"""
391+
A test to make sure we support the most basic use case of calling pylibmc.Client([url])
392+
"""
393+
url = "%s:%s" % (cfg["host"], cfg["port"])
394+
395+
patch()
396+
client = pylibmc.Client([url])
397+
assert client.addresses[0] is url
398+
Pin.get_from(client)._clone(service=self.TEST_SERVICE, tracer=self.tracer).onto(client)
399+
client.set("a", 1)
400+
spans = self.pop_spans()
401+
assert spans, spans
402+
assert len(spans) == 1
403+
374404

375405
class TestPylibmcPatchSchematization(TestPylibmcPatchDefault):
376406
@TracerTestCase.run_in_subprocess(env_overrides=dict(DD_SERVICE="mysvc", DD_TRACE_SPAN_ATTRIBUTE_SCHEMA="v0"))

0 commit comments

Comments
 (0)