Don't break memcached.getMulti with instrumentation #190
Conversation
By introducing a second server into the memcached test we demonstrate that the instrumentation doesn't correctly handle getMulti in the multi-server case.
For some calls -- in particular, getMulti -- memcached.command takes an extra argument that forces the choice of a particular server. The memcached instrumentation was eating this extra parameter and not passing it on to the wrapped implementation, which caused the library to consult only one memcached server for getMulti, giving the appearance of a cache miss for anything not on that server. Instead we now pass all of the additional arguments through verbatim, fixing the multi-server behavior and generally being a little more polite to the interface of the underlying memcached library.
|
@apparentlymart Howdy! I appreciate this work, but we already have a patch for this that I wrote last week and will be going out with the next release. I found that using |
|
Doesn't matter to me as long as it gets fixed. :) I don't see your fix in the repo, though. Am I just missing it or do you guys work in a separate internal repo? We (Say media) are currently running our forked version of the agent while we wait for this to be fixed but we'd prefer to run your patch if you can make it available. |
|
That code isn't public yet, it will be very soon. Your fix wont have any issues that I can see so running from your fork for now will be fine. I'll try to remember to comment on this ticket once the next release is out. |
chore: update explorer hub links in readme
chore: update explorer hub links in readme
Internally to
memcached.getMulti, thememcached.commandmethod is called with a second argument to force a particular server. Thememcachedinstrumentation is currently failing to pass on that parameter, which causes the library to send thegetMultirequest to only one server, which surfaces as apparent cache misses for any key not on that server.This is pretty insidious since adding the agent to an existing application that was already using
memcached.getMultiactually worsens performance (by introducing extra cache misses) but doesn't raise any obvious error to indicate that something is up.This patch fixes the issue by passing on any additional
memcached.commandarguments verbatim to the wrapped implementation.In order to test this I had to introduce a second memcached server to the integration test. What I have written here works for me having manually started up a second server, but I don't have docker installed and thus I didn't update the Makefile to start up that second memcached server.