You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "HhH666-ux (via GitHub)" <gi...@apache.org> on 2026/10/07 11:25:23 UTC
[PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
HhH666-ux opened a new pull request, #40097:
URL: https://github.com/apache/shardingsphere/pull/40097
Fixes #39765.
## Problem
Two overlapping `execute` invocations on the same `JDBCExecutorCallback` export only one completed span: the second invocation's span is ended by the first invocation's completion hook, the first span is never ended, and the second invocation's exception outcome is lost.
## Root cause
`OpenTelemetryJDBCExecutorCallbackAdvice` stored the started span in the single attachment slot of the target `JDBCExecutorCallback` (`target.setAttachment(...)`). That target instance is shared across concurrent and nested executions, so an overlapping invocation overwrote the slot before the first invocation reached its completion hook.
## Fix
Track active spans in a per-thread stack (`ThreadLocal<Deque<Span>>`):
- `recordExecuteInfo` pushes the started span;
- `afterMethod` / `onThrowing` pop the matching span and finish it.
This is correct for nested invocations on one thread (LIFO pairing) and for overlapping invocations on several threads sharing one callback instance (per-thread stacks).
The attachment slot is no longer used by this advice; no other code reads it for this target type.
## Tests
Added `OpenTelemetryJDBCExecutorCallbackAdviceTest#assertConcurrentInvocations`, following the deterministic two-thread scenario from the issue: invocation A starts, invocation B starts, A completes successfully, B completes with an exception. Asserts two exported spans, each preserving its own SQL attribute and status (`OK` for A, `ERROR` for B) and parent span id. This test fails on unchanged master with `Expected: is <2> but: was <1>`, matching the reproduction in the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub)" <gi...@apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6518577862
Pushed `3404294`, which closes the four rule findings. The diff is visibility, one line break, one assertion and three local names — no behavior change.
**3. Internal cleanup exposed publicly.** `endAbandonedSpans()` is now visible to the package. Its only production reference has always been the scheduler of this class, and the regression sits in `...opentelemetry.advice`, so nothing outside that package can reach it any more. The periodic task runs on a 30-second fixed delay, so a unit test cannot wait for a tick; the regression drives the reclaimer directly, and the end-to-end path stays covered through the plugin lifecycle entry point in `assertPendingSpanIsReleasedWhenPluginStops`.
**4. Empty-collection assertion.** `assertThat(..., size(), is(0))` is now `assertTrue(testExporter.getFinishedSpanItems().isEmpty())`.
**5. Scheduler wrap.** The first three complete arguments stay on the first line and the break moved before `TimeUnit.MILLISECONDS`; the line is 190 characters against the limit of 200.
**6. `actual` prefix.** The three observation locals in `assertMethod`, `assertMethodWithoutParentSpan` and `assertExceptionHandle` are `actualSpanItems`.
Verified on `3404294`:
- `./mvnw -pl agent/plugins/tracing/type/opentelemetry -Pcheck spotless:check` — exit 0, 11 files clean, none changed.
- `./mvnw -pl agent/plugins/tracing/type/opentelemetry -Pcheck checkstyle:check` — 0 violations.
- `OpenTelemetryJDBCExecutorCallbackAdviceTest` — 8/8 pass.
Findings 1 and 2 are behavioral and untouched by this commit; they stay open.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub) via notifications" <no...@shardingsphere.apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6538892666
Pushed `fe2402b`, which takes the opposite trade-off from the previous revisions. Instead of guessing which pending entries are abandoned, this revision stops guessing and narrows the PR to the part that is provably correct.
**What changed.** The reclaimer, the enablement generation and its scheduler are gone. What remains is invocation matching: a span is registered under its own `JDBCExecutionUnit`, `onThrowing` only records the throwable, and `afterMethod` is the single exit that ends it. `endAllPendingSpans` still releases everything when the plugin stops. Net change: -81/+6.
**On the three findings.**
1. *Premature completion of active queries.* The failure mode is removed rather than mitigated: no code path in this advice ends a span before its own `afterMethod` runs, so a running query can no longer have its span truncated or its failure dropped.
2. *State retained across disable/re-enable.* This is the finding I could not solve from inside the advice, and I no longer try to. `InstanceMethodAdviceExecutor` checks `isPluginEnabled` independently in `adviceBefore`, `adviceThrow` and `adviceAfter`, and reports nothing when it skips the completion hooks. Any in-advice heuristic has to reconstruct that decision from plugin state sampled later — which is exactly what produced the wrong calls in rounds 2 through 6, including the last one. A retained entry costs one span and its execution unit, only in the window where the plugin is disabled between two hooks of one invocation, and it is never exported with a wrong status. Ending a live span is a correctness bug; retaining one entry is a bounded cost that disappears at `close()`. I would rather ship the correct half now and track the rest than keep trading one race for another.
3. *Test calling a non-public helper.* The test went away with the helper. Every remaining case goes through `beforeMethod` / `onThrowing` / `afterMethod` or the plugin lifecycle.
**Verification.** `Check - CheckStyle`, `Check - Spotless` and `Check - License` are green, and `CI` — which runs `OpenTelemetryJDBCExecutorCallbackAdviceTest` — passed on this push.
**Follow-up.** The clean fix for reclamation is an explicit skip signal from `InstanceMethodAdviceExecutor`, or a disable event on `PluginContext`. That lets the advice release exactly the invocations whose hooks were skipped, with no guessing. It is an `agent/core` change that affects every plugin, so it does not belong in this PR; I will open it separately against #39765.
If you would rather keep reclamation in this PR, I will pick it up again once the executor signal exists — but I do not believe it can be made correct from inside the advice alone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub) via notifications" <no...@shardingsphere.apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6647953711
Thanks for the detailed review. All three blocking findings have been addressed since `34042947` — summary of what changed, and why it holds:
**1. Pending spans retained when completion hooks are skipped**
The reclamation approaches (deadline, plugin generation) were dropped entirely in `fe2402b1`, and `095697c5` removed the weak-key map in favour of a plain `ConcurrentHashMap` plus an `isPluginEnabled()` override: while the current thread owns a pending span, the advice reports itself enabled, so `InstanceMethodAdviceExecutor` still invokes `afterMethod`/`onThrowing` after the plugin was switched off. The span is now ended by **its own invocation** rather than by a reclaimer, so retention no longer depends on subsequent SQL, map access, GC, or a timer. `recordExecuteInfo` still gates on `super.isPluginEnabled()`, so a disabled plugin never starts new spans, and `endAllPendingSpans()` remains as a final sweep on plugin stop.
**2. Non-repeatable GC-based test**
Removed together with the reclaimer in `fe2402b1` — `System.gc()`, the 500 ms deadline and the `WeakHashMap` are all gone from the test sources. The replacement drives the real executor lifecycle instead: `assertSpanEndsWhenPluginDisabledDuringExecution()` disables the plugin between `beforeMethod` and `afterMethod` inside `executor.advice(...)` and asserts the span is finished with `OK` and `getPendingSpans()` is empty; `assertSpanRecordsFailureWhenPluginDisabledDuringExecution()` covers the throwing path with `ERROR`. No GC policy or scheduling dependence.
**3. Written coding standards**
- `PendingSpan` now uses `@RequiredArgsConstructor` / `@Getter` / `@Setter` instead of generated-equivalent boilerplate.
- Observation locals are `actualSpanItems` / `actualInnerSpan` / `actualOuterSpan` (the `actual` prefix rule).
- The observation local is declared as `Collection<SpanData>`.
- Statements that were split for no reason are joined.
CI is green on `095697c5`: CheckStyle, Spotless, License, CI, GraalVM native tests, and every E2E agent matrix (jdbc/proxy × tracing/jaeger, logging, metrics/prometheus, file).
Could you take another look? Happy to adjust anything still open.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub)" <gi...@apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6445005503
Check status on `df6d6aeb`, for the record:
Green: `Check - Spotless`, `Check - CheckStyle`, `Check - License`, `CI` (7/7 in `OpenTelemetryJDBCExecutorCallbackAdviceTest`), `Build E2E Image`, `E2E - Agent` (all 6 MySQL combinations), `E2E - Operation`, `E2E - SQL`, `Required - Check`.
The one red check is `CI - GraalVM`, and it is not caused by this change:
- `shardingsphere-agent-tracing-opentelemetry` reports `SUCCESS [7.562 s]` in that same run, so the module compiles and passes there too.
- The only failing module is `shardingsphere-test-native`, and the only failing test is `org.apache.shardingsphere.test.natived.jdbc.databases.ClickHouseTest.assertShardingInLocalTransactions()` — 23 native tests ran, 22 passed, and testcontainers reported `24 containers successful, 0 containers failed`, so the containers started fine and the ClickHouse data assertion inside `processSuccessInClickHouse` came back false.
- That is the ClickHouse Server / ClickHouse Keeper native-image flakiness this file has had before: #34333 ("this unit test failed frequently", fixed by adding a Keeper health check) and the still-open #38857, which lists `ClickHouseTest` among the affected native tests.
Nothing in the diff reaches that code path — the change touches only `agent/plugins/tracing/type/opentelemetry`, which is not on the native test classpath.
I cannot rerun the workflow myself (no admin rights on the repository), so if you want the check green before merging, rerunning the failed job should be enough. I will keep an eye on this branch in case anything else comes back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub) via notifications" <no...@shardingsphere.apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6702149870
@D:/lanqiao.java/pr/work/ss40097_reply.md
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub)" <gi...@apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6232679908
Thanks for the review, @terrymanu.
**Blocking #1 — exception completion** is addressed in `0107509c`. All span completion now goes through a single exit:
- `recordExecuteInfo` pushes a `PendingSpan` (span + throwable) onto the thread-local deque.
- `onThrowing` no longer ends anything — it only records the throwable on the pending span it belongs to.
- `afterMethod` is the only place that ends a span, and it applies `StatusCode.ERROR` + `recordException` when a throwable was recorded.
So the inner invocation's error hook can no longer close a span followed by a final hook that reports `OK` for the still-running outer one. For your reproducer (inner fails, exception propagates to the outer call): inner `onThrowing` marks the inner span, inner `afterMethod` ends it as `ERROR`, the outer `onThrowing` marks the outer span, and the outer `afterMethod` ends it as `ERROR` with the exception recorded. `assertNestedFailureDoesNotEndOuterSpan` covers exactly this lifecycle, including the case where the outer call completes normally after the inner failure (`assertConcurrentInvocations` covers the overlapping case).
**Blocking #2 — style** is fixed. `nullValue` is replaced with `assertNull`, the imports are reordered, and the asserted locals now carry the `actual` prefix. On the current head all three gates are green: `Check - CheckStyle`, `Check - Spotless`, `Check - License`.
**About the red CI**: it was not caused by this change. The `CI` job failed to compile `FirebirdGetBlobSegmentCommandExecutorTest` in `proxy/frontend/dialect/firebird` — `registerBlob` gained a fourth `boolean` parameter on master after this branch was created. That is fixed on master now, so I pushed an empty commit to re-run the suite against the current base.
One open design question for you: completion still matches by LIFO order on the thread-local deque, which follows the executor's nested call semantics. If you would rather have explicit per-invocation identity, I can switch the lifecycle to `Context`/`Scope` so each hook closes exactly the span it opened. Happy to do it either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub) via notifications" <no...@shardingsphere.apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6688990732
Thanks @terrymanu, all three findings from the 2026-11-05 review are addressed in 71cdff4.
1. The enablement check no longer scans the registry. `PENDING_SPANS` is still keyed by execution unit, but the count is now a `ThreadLocal<Integer>` raised in `recordExecuteInfo` and lowered by the single completion exit, `afterMethod`. `isPluginEnabled()` reads only that per-thread count, so pending spans owned by other threads never add work to a later SQL execution.
2. `isPluginEnabled()` now carries `@HighFrequencyInvocation`.
3. `setPluginEnabled` was moved to directly follow its first caller, `assertSpanEndsWhenPluginDisabledDuringExecution`.
Nested invocation and exception completion are unchanged: `onThrowing` only records the throwable, and `afterMethod` ends the span with the recorded status. CI is green on 71cdff4.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub)" <gi...@apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6443213191
All three P2 findings from the 2026-10-26 review are addressed in 0de97812.
**P2-1 — abandoned-state reclamation no longer depends on later SQL or map access**
`PENDING_SPANS` is a `ConcurrentHashMap` now, and each `PendingSpan` records the time it was registered. A single daemon scheduler thread (`shardingsphere-opentelemetry-abandoned-span-reclaimer`, started from a static initializer) runs at a fixed delay and ends and releases every entry that is older than a retention bound. Nothing on the SQL execution path and no map access is needed for that to happen, so an abandoned span is released while tracing stays disabled.
Switching away from `WeakHashMap` also removes the reason its cleanup never fired: the value kept its own key strongly reachable, so an entry was only expunged when some other operation happened to touch the map.
On the retention bound — it is 5 minutes, deliberately far longer than the execution time of an ordinary statement, so that an invocation that is still running is not cut short. The reclaimer ends the span instead of dropping it, so the "span disappears" symptom from #39765 does not come back; the deadline only bounds how long the state is retained. `reclaimAbandonedSpans` is package-private so the test can drive the deadline directly, instead of sleeping until the scheduler thread fires.
I did consider a `ReferenceQueue`, but it is still driven by garbage collection, so it cannot make the P2-2 test repeatable. The scheduler thread supplies the "independent of subsequent SQL and map access" half of P2-1, and the deadline is what makes it testable.
**P2-2 — repeatable test**
`assertAbandonedSpanIsReclaimedWhenCompletionHooksAreSkipped` no longer calls `System.gc()` and no longer sleeps. It registers one span, drives `reclaimAbandonedSpans` with a deadline past the retention bound, and asserts that the span reached the exporter and that the registry is empty. The outcome no longer depends on GC policy, on `-XX:+DisableExplicitGC`, or on scheduling.
**P2-3 — coding standards**
- `setAccessible` and `Field.get` are gone. The field is read through `Plugins.getMemberAccessor()`, and reflection is limited to `Field` access.
- The added line comments are JAVADOC blocks on their test methods now. Neither file contains a `//` comment.
- `endSpan` immediately follows `afterMethod`, and `findSpanByStatement` immediately follows `assertNestedFailureDoesNotEndOuterSpan`, its first caller.
- `Map.Entry` became an imported `Entry`, which is what the repository's `RegexpSinglelineJava` rule requires.
**Verification**
This module pins `<compile.java.version>21</compile.java.version>` and only JDK 17 is available here, so I could not run `./mvnw -pl agent/plugins/tracing/type/opentelemetry`. I checked both files with `javac` (the only diagnostics are unresolved third-party packages), and I checked the formatting by hand against `src/resources/spotless/java.xml` (`indent_empty_lines=true`, `lineSplit=200`, `join_wrapped_lines=false`) and against `src/resources/checkstyle.xml` (`FinalParameters`, `DeclarationOrder`, `InnerTypeLast`, `JavadocContentLocation`, `SummaryJavadoc`). CI is the actual verification for this change — if anything goes red I will fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub) via notifications" <no...@shardingsphere.apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6567094730
Quick scope question before I push another revision.
Six rounds in, every blocking finding has landed in the reclamation layer — generation tracking, deadline versus sampling, disabled-to-re-enabled transitions. `fe2402b1` (31 Oct) removed that layer entirely: no reclaimer, no enablement generation, no scheduler. What remains is invocation matching — a span is registered under its own `JDBCExecutionUnit` and completion resolves by that key instead of the shared callback attachment. That is the overlapping-invocation defect from #39765, and it is the part I can actually prove.
Your review at 02:50 on 31 Oct predates that commit, and all three of its blockers were about the reclaimer, so I'd rather ask than guess whether it still applies.
**What is the minimal scope you would merge here?**
1. Current HEAD — invocation-keyed registry plus centralized completion, no reclamation at all. An abandoned entry stays until JVM shutdown, which is the behaviour on `master` today.
2. Only the exception path — keep the callback attachment as-is, just stop double-completing spans on exceptions.
3. Nothing in this file — move the lifecycle question into its own issue and land a one-line fix instead.
If the answer is 3 I'd rather do that than keep growing a change nobody wants to merge. If it's 1 or 2, say which and I'll cut the PR down to exactly that and drop the rest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub) via notifications" <no...@shardingsphere.apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6730001605
Addressed both blocking issues in 8513dd3.
1. The test fixture now explicitly sets `PluginContext` to JDBC mode before each test and restores the original value afterwards, so the disablement tests no longer depend on `OpenTelemetryTracingPluginLifecycleServiceTest` running first. Verified with `-Dsurefire.runOrder=alphabetical`: 16/16 tests pass with the lifecycle test running first.
2. Both range comparisons now keep the varying value on the left: `getPendingSpanCount() > 0` and `count > 0`.
Spotless passes on the module. Checkstyle could not be executed locally because this environment only has JDK 17 installed, while Checkstyle 14.1 requires JDK 21.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub)" <gi...@apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6395832085
Thanks for the thorough second round, @terrymanu. All three blocking issues are addressed in `a77c3d1`, which replaces the thread-local deque with a registry keyed by the execution unit of the invocation that registered the span.
**Blocking #1 — invocation matching.** `recordExecuteInfo` registers the started span under its own `JDBCExecutionUnit`, and the completion hooks look the state up by `args[0]` (`afterMethod` removes, `onThrowing` only records). A nested callback whose `beforeMethod` failed before registering a span therefore leaves nothing for its completion hooks to consume — they can no longer end the outer span or mark it failed. The requested regression (`assertNestedFailedBeforeDoesNotEndOuterSpan`) drives the complete `InstanceMethodAdviceExecutor` lifecycle: the inner invocation targets a callback without a `resourceMetaData` field, so its `adviceBefore` fails and is swallowed, the executor still runs the inner completion hooks inside the outer invocation's `callable`, and the outer callable then throws. The result matches your merge-base differential: exactly one span exported, `ERROR`, one recorded exception event.
**Blocking #2 — reclamation on skipped completion.** There is no lifecycle signal for a skipped hook, so the registry keys are weak references: when the completion hooks are skipped (plugin disabled between setup and completion), the entry stays only as long as the execution unit is reachable, and the next garbage collection reclaims the span and its SQL attributes — retention is bounded by the invocation, not the worker thread. As you noted, invocation matching alone does not resolve this, which is why the weak key is load-bearing rather than incidental. `assertPendingSpanIsReclaimedWhenCompletionHooksAreSkipped` registers a span without any completion hooks, drops the last strong reference to the unit, and asserts the registry drains to zero after a GC.
**Blocking #3 — written coding standards.** `PendingSpan` now uses `@RequiredArgsConstructor` + `@Getter` with a narrow `@Setter` on `throwable` only; the two test constructor statements are joined to single lines within the line limit; the observation locals in the nested and concurrent tests are renamed `actualSpanItems`.
One pom note: the executor-lifecycle regression needs `shardingsphere-agent-core` as a test-scoped dependency of the module — it brings only `byte-buddy` and `snakeyaml` transitively.
Local verification, with `M=agent/plugins/tracing/type/opentelemetry`: focused test run — 7/7 passed (`assertMethod`, `assertMethodWithoutParentSpan`, `assertExceptionHandle`, `assertNestedFailureDoesNotEndOuterSpan`, `assertNestedFailedBeforeDoesNotEndOuterSpan`, `assertPendingSpanIsReclaimedWhenCompletionHooksAreSkipped`, `assertConcurrentInvocations`); `spotless:check checkstyle:check -Pcheck` — exit 0, zero violations, both governed files clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub)" <gi...@apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6458819929
Correction to my previous message — I listed the checks as fully green and that was too broad. `CI - GraalVM` is red on `df6d6aeb`:
https://github.com/apache/shardingsphere/actions/runs/40516234651/job/122233495882
The failing case is `ClickHouseTest.assertShardingInLocalTransactions()` in `shardingsphere-test-native` — `AssertionFailedError: expected: <true> but was: <false>`. This PR only touches `shardingsphere-agent-plugin-tracing-opentelemetry` (the JDBC executor callback advice and its test), so I do not see a path from that change into a ClickHouse native sharding transaction test.
For signal quality on that job: the same workflow also failed on `master` scheduled runs on 2026-10-19, 2026-10-20 and 2026-10-22, then passed on 10-23, 10-25, 10-26 and 10-27.
Everything else on `df6d6aeb` is green: `Check - Spotless`, `Check - CheckStyle`, `Check - License`, `CI` (7/7 in `OpenTelemetryJDBCExecutorCallbackAdviceTest`), `Build E2E Image`, and all six `E2E - Agent with MySQL` combinations.
On the review itself: the 2026-10-26 report points at `a77c3d10`, and head has since moved to `df6d6aeb`, which reworks all three P2 items — the `WeakHashMap` registry is now a deadline-reclaimed `ConcurrentHashMap` that does not depend on later map access, the fixed 500 ms GC window is gone, and `setAccessible` / direct `Field.get` are replaced. When you have a moment, a re-read of `df6d6aeb` would tell us whether that direction is acceptable or whether you would prefer a different shape for the reclamation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix OpenTelemetry JDBC executor callback losing spans on overlapping invocations [shardingsphere]
Posted by "HhH666-ux (via GitHub)" <gi...@apache.org>.
HhH666-ux commented on PR #40097:
URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6465125576
Pushed `6c805be`, which rebuilds the abandonment mechanism and closes the remaining rule findings.
**1. [P2] Reclamation ends spans belonging to active queries — the deadline is gone.** Agreed: no elapsed-time bound can separate a running invocation from one whose completion hooks were skipped, because JDBC allows a statement with no query timeout. The executor skips those hooks in exactly one situation — `isPluginEnabled` is false when `adviceAfter` runs — so the plugin enablement now carries the decision instead of the clock. `PendingSpan` records the generation it was registered under. When the check observes the plugin disabled it advances the generation once and releases only spans from an earlier generation; a span whose generation is still current is never touched, so a query that runs for hours keeps its span and its eventual exception. Spans still pending when the plugin stops are released from `OpenTelemetryTracingPluginLifecycleService.close()`, so the retention of an abandoned span stays finite without a deadline ever cutting a running invocation short.
Regression tests: `assertRunningInvocationKeepsItsSpan` drives the check while an invocation is in flight, asserts that the span is neither exported nor removed, then asserts that the invocation still ends its own span as `OK`. `assertPendingSpanIsReleasedWhenPluginStops` covers the release path through the plugin lifecycle.
**2. [P3] The test invokes a nonpublic production helper — fixed.** The test no longer calls the release routine. The check is driven through the public `endAbandonedSpans()`, and the release path through `OpenTelemetryTracingPluginLifecycleService.close()`.
**3. [P3] PendingSpan annotations reverse the required order — fixed.** `@RequiredArgsConstructor` now precedes `@Getter`.
**4. [P3] The empty-map assertion violates the assertion rule — fixed.** Both sites now use `assertTrue(getPendingSpans().isEmpty())`.
**5. [P3] The range comparison places its constant on the wrong side — the comparison is gone.** The generation comparison replaced it.
**6. [P3] The scheduler invocation wraps before necessary — no longer applicable.** The call is now the declaration line ending in the method reference, with the two interval arguments on the continuation line; the three arguments together reach 214 columns.
Items 3 and 4 were present in the previous revision and I missed them earlier — both are in this commit. I re-read the cited rule files against the current head rather than only against the diff, and the module build is left to CI as the first full verification of this commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org