Report configured Couchbase Protostellar targets - #20089
Draft
trask wants to merge 3 commits into
Draft
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-09-10 22:01 UTC Move out of draft to request review. Status above doesn't look right?
|
trask
added this pull request to stack #20052
September 10, 2026 03:21
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The explicitly configured non-default port behavior lacks direct test coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds configured-target reporting for Couchbase Protostellar (couchbase2://) requests.
Changes:
- Propagates targets through dynamically registered virtual fields.
- Treats port
18098as the Protostellar default. - Adds latest-dependency integration coverage.
File summaries
| File | Description |
|---|---|
CouchbaseServerTarget.java |
Defines the Protostellar default port. |
CouchbaseConnectionStrings.java |
Parses couchbase2 as a direct target. |
CouchbaseConfiguredTarget.java |
Supports request-level target fallback. |
CouchbaseClient32Test.java |
Tests default Protostellar target reporting. |
CouchbaseRequestTracer.java |
Retrieves request-associated targets. |
CouchbaseProtostellarTargets.java |
Manages dynamic virtual-field associations. |
CouchbaseProtostellarRequestInstrumentation.java |
Associates requests with cores. |
CouchbaseProtostellarCoreInstrumentation.java |
Captures configured core targets. |
CouchbaseInstrumentationModule.java |
Registers instrumentation and virtual fields. |
build.gradle.kts |
Adds compile-only @NoMuzzle support. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot comment: [Testing] This exercises only the implicit `18098` path, while the newly promised non-default-port behavior is not covered. Add a latest-deps stable-semconv case using a URI such as `couchbase2://<seed>:18099` and assert that `SERVER_PORT` is `18099`; otherwise a regression that drops all Protostellar ports would still pass. Analysis: The existing test covered only a connection string without an explicit port. Parameterizing it with `:18099` proves that target parsing and span capture preserve a non-default Protostellar port while retaining default-port omission coverage. Upsides: The test now catches regressions that discard explicit Protostellar ports and also verifies the port-bearing stable span name. Downsides: The latest-dependency test runs one additional parameterized invocation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… target test Review finding: The two Mockito stubbings on the mocked ProtostellarRequest are dead code: nothing in the exercised path ever calls createdAt() or serviceType(). Verified against core-io 3.12.2 bytecode and the instrumentation sources: ProtostellarBaseRequest(CoreProtostellar, ProtostellarRequest) only assigns the two fields; ProtostellarBaseRequest.context() builds new RequestContext(null, id, env, auth, this) and RequestContext's constructor only stores the request; CouchbaseRequestTracer.requestContext() calls CouchbaseConfiguredTarget.capture() and CouchbaseProtostellarTargets.getRequestTarget(), which read only requestContext.core() and a VirtualField; CouchbaseSpanName never touches the request; CouchbaseSpan and CouchbaseTracer contain no reference to a request at all. Only couchbase-3.0's tracer calls request.serviceType(), and that tracer is not the one installed for 3.2. Because the mock is created with plain Mockito.mock() rather than MockitoExtension, lenient strictness silently accepts the unused stubs, so nothing fails. Fix: delete both when(...) statements, keeping Object protostellarRequest = mock(protostellarRequestClass). That also removes the reflective when(Method.invoke(mock)) idiom, which is hard to read because the stubbed call is expressed as a reflective invocation rather than a direct method call, and it lets the now-unused imports be removed: static org.mockito.Mockito.when and com.couchbase.client.core.service.ServiceType. Analysis: The test builds a ProtostellarBaseRequest around a mocked ProtostellarRequest, takes its RequestContext, and drives the OpenTelemetry RequestSpan by hand. Bytecode for core-io 3.12.2 shows the ProtostellarBaseRequest constructor stores only the core and the request, and context() builds a RequestContext whose constructor stores the request without calling it. The couchbase-3.2 span path reads requestContext.core(), requestContext.request(), and the ProtostellarBaseRequest virtual field, so it never reaches createdAt() or serviceType() either. The mock is created with a bare Mockito.mock() call, outside any MockitoExtension or MockitoSession, so unused-stub detection never runs and the dead stubs stay invisible. A floating latestDepTestLibrary version does not make them live: a future SDK that starts calling those methods would need its own change, and the mock already returns Mockito defaults for both. Upsides: Removes setup that looks load-bearing but is not, so a reader no longer has to work out which SDK call consumes it. Drops the reflective when(Method.invoke(mock)) idiom, in which the stubbed call appears as a reflective invocation instead of a direct method call. Removes two imports that no longer have a use. Downsides: If a future Couchbase SDK version starts calling createdAt() or serviceType() while the request or its RequestContext is being built, the mock returns Mockito defaults (0 and null) rather than the values the stubs supplied. Neither value reaches an assertion in this test, and a version that behaves that way would need its own change anyway. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
trask
force-pushed
the
trask-couchbase-protostellar-targets
branch
from
September 10, 2026 15:33
a4595fc to
69b9326
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cloud Native Gateway requests made through
couchbase2://now report the configured endpoint inserver.address. Port 18098 is treated as the scheme default, so it is omitted fromserver.port; an explicitly configured non-default port is still reported.CoreProtostellarandProtostellarBaseRequestare absent from the module's minimum Couchbase 3.2 dependency, so their configured-target associations use dynamically registered virtual fields without raising the supported version floor.