Skip to content

eval: jedis 1.4 instrumentation (blind test, config v1) #10917

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

Draft
jordan-wong wants to merge 2 commits into
base: master
Choose a base branch
from
+178 −1,566
Draft

eval: jedis 1.4 instrumentation (blind test, config v1) #10917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4215789
eval: prepare blind test for jedis
jordan-wong Mar 20, 2026
5ce4223
eval: add generated jedis 1.4 instrumentation (blind test, config v1)
jordan-wong Mar 20, 2026
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
54 changes: 54 additions & 0 deletions .claude/skills/add-apm-integrations/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,3 +230,57 @@ After the instrumentation is complete (or abandoned), review the full session an

Keep each change minimal and targeted. Do not rewrite sections that worked correctly.
After editing, confirm to the user which improvements were made to the skill.

## Appendix: Collected review rules

These rules are derived from expert feedback on generated PRs. They serve as a checklist for self-review before finishing. Some overlap with guidance in the steps above; they are collected here for completeness.

### R1: No lambdas in advice classes (error)
Search for lambda expressions (`->` or `::`) in any file with "Advice" or "Instrumentation" in its name. Advice methods are inlined into bytecode by ByteBuddy, and lambdas create invokedynamic instructions that break when inlined into a different classloader context.
*Source: mcculls, PR #10579*

### R2: Assign wrapped future back with `@Advice.Return(readOnly=false)` (error)
In async advice exit methods that wrap futures (CompletableFuture, ListenableFuture, etc.), verify the wrapped result is assigned back to the return value using `@Advice.Return(readOnly=false)`. Do not discard the return of `future.whenComplete`/`thenApply`/etc.
*Applies to: async instrumentations only. Source: mcculls, PR #10579*

### R3: Single InstrumenterModule per integration (error)
There should be exactly one class extending `InstrumenterModule` (with `@AutoService(InstrumenterModule.class)`) per module. If the integration needs to instrument multiple classes, list them as separate `Instrumenter` implementations within the same module.
*Source: mcculls, PR #10579*

### R4: Module name must end with version (error)
The module directory under `dd-java-agent/instrumentation/` must follow `{library}-{version}` (e.g., `feign-8.0`, `okhttp-3`). Do not use the Maven artifact name (e.g., `feign-core`).
*Source: mcculls, PR #10579*

### R5: CallDepthThreadLocalMap must be reset on the same thread (error)
If `CallDepthThreadLocalMap` is used for reentrancy protection, `increment()` and `reset()` must be called on the same thread. In async code paths, callbacks may run on a different thread, so `CallDepthThreadLocalMap` cannot be used across async boundaries.
*Applies to: async instrumentations only. Source: mcculls, PR #10579*

### R6: Code passes codeNarc checks (warning)
If there are Groovy test files, avoid common codeNarc violations: unused imports, unnecessary semicolons, missing spaces after keywords. Run `codenarcTest` in addition to `spotlessCheck`.
*Source: Runs 0, 1*

### R7: Test against minimum supported version, not just latest (warning)
Compare the `testCompile`/`testImplementation` dependency version in `build.gradle` against the minimum version in the muzzle `pass` directive. If the test uses APIs that only exist in newer versions, it does not actually verify minimum version compatibility.
*Source: Run 1 (auto-detected)*

### R8: Test extends correct base test class (warning)
The test superclass should match the integration type:
- HTTP clients: extend `HttpClientTest`
- HTTP servers: extend `HttpServerTest`
- Database clients: extend `DatabaseClientTest`
- Messaging: use the appropriate messaging base test

If no base test class exists for the type, custom tests are acceptable but should cover span creation, error handling, and tag verification.
*Source: Run 1 (auto-detected)*

### R9: Disabled base test methods must have justification (warning)
Methods that override base test class methods to return `false` (e.g., `testRedirects() { return false }`) must have a comment explaining why — what specific library limitation prevents the test from working.
*Source: Run 1 (auto-detected)*

### R10: Test exercises real library, not just mocks (error)
The test must instantiate real library objects and make real calls (e.g., a real HTTP client calling a test server). Tests that only use mocks without exercising actual library code do not verify the instrumentation works. Base test classes typically provide real server infrastructure.
*Source: PR #10317 (Resilience4j)*

### R11: Test compiles and runs against minimum supported version (warning)
Verify that the test code only uses APIs available in the minimum supported version. If the test uses a newer API (e.g., a static factory method added in a later version), it cannot verify minimum version compatibility. This extends R7 with API-level checking.
*Source: Run 1 (extends R7)*
52 changes: 26 additions & 26 deletions components/json/src/test/java/datadog/json/JsonMapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@

class JsonMapperTest {
@TableTest({
"Scenario | Input | Expected ",
"null input | | '{}' ",
"empty map | [:] | '{}' ",
"single entry | [key1: value1] | '{\"key1\":\"value1\"}' ",
"two entries | [key1: value1, key2: value2] | '{\"key1\":\"value1\",\"key2\":\"value2\"}' ",
"quoted entries | [key1: va\"lu\"e1, ke\"y2: value2] | '{\"key1\":\"va\\\"lu\\\"e1\",\"ke\\\"y2\":\"value2\"}'"
"Scenario | Input | Expected ",
"null input | | '{}' ",
"empty map | [:] | '{}' ",
"single entry | [key1: value1] | '{\"key1\":\"value1\"}' ",
"two entries | [key1: value1, key2: value2] | '{\"key1\":\"value1\",\"key2\":\"value2\"}' ",
"quoted entries | [key1: va\"lu\"e1, ke\"y2: value2] | '{\"key1\":\"va\\\"lu\\\"e1\",\"ke\\\"y2\":\"value2\"}'"
})
@ParameterizedTest(name = "test mapping to JSON object: {0}")
@MethodSource("testMappingToJsonObjectArguments")
Expand Down Expand Up @@ -94,12 +94,12 @@ void testMappingToMapFromNonObjectJson(String json) {
}

@TableTest({
"Scenario | Input | Expected ",
"null input | | '[]' ",
"empty list | [] | '[]' ",
"single value | [value1] | '[\"value1\"]' ",
"two values | [value1, value2] | '[\"value1\",\"value2\"]' ",
"quoted values | [va\"lu\"e1, value2] | '[\"va\\\"lu\\\"e1\",\"value2\"]'"
"Scenario | Input | Expected ",
"null input | | '[]' ",
"empty list | [] | '[]' ",
"single value | [value1] | '[\"value1\"]' ",
"two values | [value1, value2] | '[\"value1\",\"value2\"]' ",
"quoted values | [va\"lu\"e1, value2] | '[\"va\\\"lu\\\"e1\",\"value2\"]'"
})
@ParameterizedTest(name = "test mapping iterable to JSON array: {0}")
void testMappingIterableToJsonArray(List<String> input, String expected) throws IOException {
Expand All @@ -111,12 +111,12 @@ void testMappingIterableToJsonArray(List<String> input, String expected) throws
}

@TableTest({
"Scenario | Input | Expected ",
"null input | | '[]' ",
"empty array | [] | '[]' ",
"single element | [value1] | '[\"value1\"]' ",
"two elements | [value1, value2] | '[\"value1\",\"value2\"]' ",
"escaped quotes | [va\"lu\"e1, value2] | '[\"va\\\"lu\\\"e1\",\"value2\"]'"
"Scenario | Input | Expected ",
"null input | | '[]' ",
"empty array | [] | '[]' ",
"single element | [value1] | '[\"value1\"]' ",
"two elements | [value1, value2] | '[\"value1\",\"value2\"]' ",
"escaped quotes | [va\"lu\"e1, value2] | '[\"va\\\"lu\\\"e1\",\"value2\"]'"
})
@ParameterizedTest(name = "test mapping array to JSON array: {0}")
void testMappingArrayToJsonArray(String ignoredScenario, String[] input, String expected)
Expand All @@ -137,14 +137,14 @@ void testMappingToListFromEmptyJsonObject(String json) throws IOException {
}

@TableTest({
"Scenario | input | expected ",
"null value | | '' ",
"empty string | '' | '' ",
"\\b | '\b' | '\"\\b\"'",
"\\t | '\t' | '\"\\t\"'",
"\\f | '\f' | '\"\\f\"'",
"a | 'a' | '\"a\"' ",
"/ | '/' | '\"\\/\"'"
"Scenario | input | expected ",
"null value | | '' ",
"empty string | '' | '' ",
"\\b | '\b' | '\"\\b\"'",
"\\t | '\t' | '\"\\t\"'",
"\\f | '\f' | '\"\\f\"'",
"a | 'a' | '\"a\"' ",
"/ | '/' | '\"\\/\"'"
})
@ParameterizedTest(name = "test mapping to JSON string: {0}")
@MethodSource("testMappingToJsonStringArguments")
Expand Down
Loading
Loading