Repository navigation
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-10-20 18:53 UTC Move out of draft to request review. Status above doesn't look right?
|
Co-authored-by: trask <218610+trask@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The bounded classifier preserves existing fallbacks, has focused coverage, and has no identified blocking issues.
No open issues 🎉
What changed in this PR
Improves Redisson error telemetry by classifying generic server errors with known Redis protocol codes while preserving exception-class fallbacks.
Changes:
- Restricts classification to decoder-marked generic exceptions and known prefixes.
- Adds unit coverage for classification and fallbacks, plus span and duration-metric coverage for
WRONGTYPE.
| File | Description |
|---|---|
| instrumentation/redisson/redisson-common-3.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/redisson/AbstractRedissonClientTest.java | Verifies WRONGTYPE span and metric attributes. |
| instrumentation/redisson/redisson-common-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/redisson/common/v3_0/RedissonDbAttributesGetter.java | Classifies known generic server errors. |
| instrumentation/redisson/redisson-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/redisson/common/v3_0/RedissonDbAttributesGetterTest.java | Tests classification and exception fallbacks. |
| instrumentation/redisson/redisson-3.0/javaagent-unit-tests/build.gradle.kts | Adds the common instrumentation test dependency. |
Review effort: Balanced
| } | ||
|
|
||
| String errorType = message.substring(0, separator); | ||
| switch (errorType) { |
There was a problem hiding this comment.
@copilot Update this existing PR directly with the following consolidated changes. Do not create another PR or work on other PRs.
The user explicitly decided: "I'm not worried about low cardinality here as long as we know they are error codes coming back from the server." Remove the Redis error-prefix allowlist and the formatted-exception-message heuristic based on ". channel:". Accept custom and newly introduced server error codes. The requirement is confirmed server origin, not membership in a fixed list.
Investigate Redisson's protocol decoder and find a safe instrumentation point that captures the prefix from an actual server error reply before failure callbacks complete the instrumentation operation. Redisson's CommandDecoder recognizes RESP '-' error replies, but generic RedisException has no structured error-code field and its message includes diagnostic suffixes; do not infer server origin by parsing those suffixes or arbitrary exception messages. Verify the concrete implementations for the supported redisson-3.0 and redisson-3.17 ranges, including current supported releases. Capture only the first server error-prefix token for error.type, not message text. Preserve retry behavior, completion ownership, asynchronous/batch behavior, and resource cleanup. For errors without a reliably captured server code, preserve the shared canonical exception-class fallback. Do not add an exception-class-to-code enumeration or a cardinality-motivated restriction. Prefer the simplest reliable solution. If no safe compatible decoder integration is feasible, stop and report that blocker rather than substituting another message heuristic.
Update the relevant unit and installed-agent integration tests under instrumentation/redisson. Exercise real WRONGTYPE and custom server error prefixes absent from the current allowlist (for example through a Lua error reply), client-side failures, and successful operations. Verify error.type on spans and db.client.operation.duration, including asynchronous completion and supported baseline/latest versions where relevant to the new decoder integration. Ensure no stale captured code leaks to another operation and no client failure is misclassified as a server reply.
Document briefly the server-reply capture source, any non-obvious lifecycle constraint, and exception-class fallback, with a Redis protocol or upstream decoder reference where useful; do not inventory codes. Follow repository contribution and path instructions. Run narrow formatting and relevant targeted tests in the cloud and report unavailable validation honestly. Commit and push to this PR's existing head branch, preserve its draft state, and update the title or description only where stale or misleading. Do not post reviewer replies or resolve threads.
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
|
|
||
| dependencies { | ||
| library("org.redisson:redisson:3.17.0") | ||
| latestDepTestLibrary("org.redisson:redisson:3.+") // documented limitation: Redisson 4.x is not covered by this test |
There was a problem hiding this comment.
@copilot Complete the remaining work on this existing PR directly. The previous cloud run delivered RESP decoder-level error-prefix capture at d88c597, but it restricted latest-dependency testing to Redisson 3.x and left the original PR description stale. Do not create another PR or work on other PRs.
Restore current latest-release compatibility coverage for instrumentation/redisson/redisson-3.17/javaagent instead of excluding Redisson 4.x. Investigate and address compatibility issues tightly coupled to this PR's decoder integration and tests. The new latestDepTestLibrary("org.redisson:redisson:3.+") restriction and its limitation comment must not hide an unresolved latest-release problem; restore the ordinary latest-dependency selection and remove the added cached 3.+ entry in .github/config/latest-dep-versions.json if it is no longer used. Keep coverage of the supported redisson-3.0 and redisson-3.17 baselines. Verify the actual decoder and completion paths in current Redisson 4.x rather than assuming compatibility from method names or a successful build. Make only fixes needed for this error-capture work and its compatibility coverage. If a genuine unrelated library incompatibility prevents safe coverage, report concrete evidence and the blocker rather than silently pinning, skipping, or narrowing coverage again.
Preserve the user's settled behavior: capture error prefixes only from actual server error replies, including custom/future codes without an allowlist or cardinality-motivated restriction. Do not restore ". channel:" parsing or classify arbitrary exception messages as server codes. Preserve client-side exception-class fallback, successful-operation behavior, retries, asynchronous/batch completion ownership, scoped-state restoration, and cleanup. Run narrow formatting tasks immediately before targeted validation in the cloud. Exercise WRONGTYPE, a real custom Lua error reply, client-side failures, and success against relevant baselines and current latest, checking error.type on spans and db.client.operation.duration, including asynchronous completion. Record exact validation commands and results in the cloud session result, not a testing checklist in the PR body. Report any unavailable validation honestly.
Update the original PR description to reflect the final implementation. Remove the stale claims about "bounded" or "known" codes and extraction from decoder-marked generic RedisException messages. Explain concisely that generic server error codes are captured from RESP replies before completion, with exception-class fallback where no server code is captured. Inspect and follow the repository's PR template and description instructions, preserving required template content; update the title only if misleading. If cloud cannot edit the original PR body, report that specific blocker, do not claim the metadata was updated. Commit and push intended code changes to this PR's existing head branch and preserve its draft state and base. Do not post reviewer replies or resolve threads.
Co-authored-by: trask <218610+trask@users.noreply.github.com>
| import org.redisson.client.RedisException; | ||
| import org.redisson.client.RedisLoadingException; | ||
|
|
||
| class RedissonDbAttributesGetterTest { |
There was a problem hiding this comment.
@copilot Update this existing PR #20779 directly. Commit and push only to this PR's existing head branch, preserve its draft state and base, and do not create a follow-up PR, dispatch tasks, or work on other PRs.
In instrumentation/redisson/redisson-common-3.0 and the redisson-3.0/redisson-3.17 agent modules, accept dedicated RedisException subclasses in RedissonErrorCapture capture, lookup, and cleanup, while retaining decoder-confirmed server provenance. Align token delimiting with the common contract, including removing tab as an extra separator and stopping raw RESP parsing at CRLF. Add real dedicated-server-error integration coverage and failed-batch coverage, alongside generic/custom replies, asynchronous completion, client fallback, success, and state cleanup. Verify the supported baseline and latest Redisson decoder/completion implementations, including the existing latest-release range. Do not restore formatted-message heuristics or exception-class-to-code enumerations.
Common classification contract: For a confirmed Redis server-error reply, record the first nonempty, ASCII-space-delimited token exactly as supplied. Preserve case and punctuation. Impose no allowlist, uppercase-only validation, character validation, or 64-character limit. Raw RESP parsing must also stop at the reply's CRLF terminator. Client-side failures and server replies without a usable prefix retain canonical exception-class fallback. Successful operations omit error.type. Spans and db.client.operation.duration metrics must agree. Do not parse arbitrary throwable messages as proof of server origin. Do not add configuration or a shared Gradle module.
Coverage and order: Fix the contract in tests first, then change production code only where this assignment requires it. Use the same representative cases across clients: WRONGTYPE, a dedicated server code where the client represents one, punctuation such as Custom.Code!, lowercase, a prefix longer than 64 characters, prefix-only replies, missing or empty prefixes, client failures, and success. Tabs are not an additional token separator. Keep parser edge cases in focused version-local unit tests, and use integration tests for real server provenance and emitted telemetry. Assert both span and duration-metric labels, exception-class fallback, and omission on success. Do not introduce cross-module test infrastructure solely for this matrix.
Validation and compatibility: Read CONTRIBUTING.md, the knowledge index, and applicable path instructions. Run the narrowest affected spotlessApply tasks directly before targeted behavioral validation. Run relevant integration, compatibility, latest-dependency, and Muzzle checks where advice changes; do not narrow supported/latest version coverage to hide a failure. Use the repository's existing test tasks, no clean or forced reruns. Report exact validation commands and results or concrete unavailable-validation blockers in the cloud session result, not a PR testing checklist. Compare this client's final behavior against the common contract, including span/metric agreement and preserved client-failure fallback. Preserve span boundaries, operation naming, completion ownership, retry behavior, application results, and cleanup. Keep the implementation focused. Do not post reviewer replies or resolve threads.
Do not edit this PR's title or description. Those metadata edits are handled separately by the coordinating session once coding is ready. Report coding delivery and any blockers in the cloud session result; no inline reply is required.
Co-authored-by: trask <218610+trask@users.noreply.github.com>
trask
changed the title
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Preserve both Redis prefix test contracts and move the new Redisson helper test to the current unitTests harness. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
…acks Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Record Redis server-error prefixes as
error.typeon Redisson spans and database duration metrics, including errors mapped to dedicated exception subclasses, such asOOM.Capture prefixes directly from RESP error replies before completion callbacks run. Preserve case and punctuation without an allowlist or length cap; client-side failures and replies without a captured prefix retain exception-class fallback.