Capture command arguments in spymemcached db query text#19258
Capture command arguments in spymemcached db query text#19258bhuvan-somisetty wants to merge 3 commits into
Conversation
Spymemcached spans previously carried only the operation name; the command arguments were never captured because the advice only read the method name. Capture the arguments of the instrumented MemcachedClient methods and build the query text from them, so that db.statement (db.query.text under stable semconv) is emitted, e.g. 'get my-key', 'set my-key 3600 ?' and 'getBulk key1 key2'. Stored values are masked, controlled by otel.instrumentation.spymemcached.query-sanitization.enabled, which falls back to the common otel.instrumentation.common.db.query-sanitization.enabled.
There was a problem hiding this comment.
Pull request overview
Adds sanitized memcached command arguments to database query-text span attributes.
Changes:
- Captures method arguments and constructs bounded query text with value masking.
- Adds configurable sanitization with common database fallback.
- Extends integration coverage and adds focused unit tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
settings.gradle.kts |
Registers the unit-test module. |
instrumentation/spymemcached-2.12/README.md |
Documents sanitization configuration. |
instrumentation/spymemcached-2.12/metadata.yaml |
Defines sanitization settings. |
instrumentation/spymemcached-2.12/javaagent/src/test/java/.../SpymemcachedTest.java |
Verifies emitted query text. |
.../SyncCompletionListener.java |
Passes command arguments into requests. |
.../SpymemcachedRequest.java |
Stores operation and query text separately. |
.../SpymemcachedQueryText.java |
Builds, masks, and truncates query text. |
.../SpymemcachedAttributesGetter.java |
Exposes query text as a DB attribute. |
.../OperationCompletionListener.java |
Forwards operation arguments. |
.../MemcachedClientInstrumentation.java |
Captures all advised method arguments. |
.../GetCompletionListener.java |
Forwards get arguments. |
.../BulkGetCompletionListener.java |
Forwards bulk-get arguments. |
.../SpymemcachedQueryTextTest.java |
Tests query construction behavior. |
.../javaagent-unit-tests/build.gradle.kts |
Configures unit-test dependencies. |
| return new AdviceScope<>( | ||
| handler, | ||
| callDepth, | ||
| handler.create(Context.current(), client.getConnection(), methodName)); | ||
| handler.create(Context.current(), client.getConnection(), methodName, args)); |
There was a problem hiding this comment.
Good catch, this is a real problem that this PR introduced. Before this change handler.create only built the request from the method name and could not realistically throw. Now it iterates the argument collections and calls String.valueOf on application objects, so an application toString can throw. The enter advice suppresses it, the call depth is never released and every following memcached call on that thread is treated as nested, which silently disables tracing for that thread.
Fixed in 4945e65 by restoring the call depth before rethrowing. The exit advice now also tolerates a null AdviceScope, which is what it receives when the enter advice was suppressed.
Pull request dashboard statusStatus last refreshed: 2026-07-22 01:50:12 UTC.
This automated status or its linked feedback items may be incorrect. If something looks wrong, report it with the result you expected. |
The blocking getBulk methods return a Map and were therefore not advised; the span was started by the asyncGetBulk method they delegate to, which only receives an iterator over the keys. Reading it would consume it before the instrumented method gets to it, so no keys ended up in the query text. Advise getBulk as well, so that the span starts at the call the application made, where the keys are still a collection or an array. Building the query text reads application provided arguments and may throw. The enter advice suppresses that, so the call depth has to be restored, otherwise every following call on the thread is treated as nested and is not traced.
|
@laurit @trask when you have a moment, could you take a look at this one? All checks are green and the Copilot comment is addressed. One part is worth a second opinion. The blocking The number of spans is unchanged, but for a blocking |
Fixes #18337.
The spymemcached instrumentation recorded
db.operation.namebut never captured the command arguments, so the query text attribute was always absent. The advice classes only read the method name via@Advice.Origin("#m"); the arguments were never passed along.This captures the arguments of the instrumented
MemcachedClientmethods and builds the query text from them, sodb.statement(db.query.textunder stable semconv) is now emitted:asyncGet("my-key")get my-keyset("my-key", 3600, "my-value")set my-key 3600 ?asyncGetBulk(asList("key1", "key2"))getBulk key1 key2Details:
set,add,replace,append,prependandcasare masked with?, since they hold user data. Keys and expiration times are kept.Iteratorof keys emit only the operation name, as reading the keys would consume the iterator before the instrumented method gets to it.Masking is controlled by a new setting, following the pattern used by cassandra:
otel.instrumentation.spymemcached.query-sanitization.enabled=trueIt falls back to
otel.instrumentation.common.db.query-sanitization.enabledand defaults totrue.SpymemcachedRequest.getQueryText()previously held the method name rather than a query text, which made the naming misleading once a real query text was added, so the operation name is now computed on construction and kept in its own accessor.Existing span assertions are extended with the expected query text, and the query text building is covered by unit tests in a new
javaagent-unit-testsmodule.