Repository navigation
[ISSUE #8208] Observe actual selector and rule matching decisions without reevaluation - #8291
Conversation
…ns without reevaluation
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
PR #8291 adds opt-in SelectorRuleMatchObserver/Observation to AbstractShenyuPlugin decision branches to trace matched selector/rule and cache hits (fixes #8208). Head SHA 57f6efb. e2e checks green (truncated). Bounded patches; no full review.
Evidence
- PR #8291 head SHA 57f6efb; files: AbstractShenyuPlugin.java, SelectorRuleMatchObservation.java, SelectorRuleMatchObserver.java.
- Checks: e2e and e2e-case (springcloud, websocket, logging-rocketmq, grpc) success (list truncated).
- Source snapshot 57f6efb includes excerpt of SelectorRuleMatchObservation.java describing immutable decision summary.
Recommended next steps
- Verify full patch for thread-safety and no perf regression on AbstractShenyuPlugin hot path.
- Confirm opt-in static registration matches existing diagnostics/metrics pattern and docs are updated.
- Run unit/integration tests beyond e2e to catch observer regressions.
Scope: bounded repository evidence; this is not a full code review or a claim that tests were run.
Task: aa3ffe97-6748-4d19-b00b-5f068d4b78ea · Commit: 57f6efb4e58465715856e1f97cb4e47ffe9147e7
|
Thanks @Aias00 for the review. On the three follow-up suggestions:
|
wy471x
left a comment
wy471x
left a comment
There was a problem hiding this comment.
Review: opt-in selector/rule match observation
The reporting placement is careful and I could not find a branch that reports twice or not at all. The extraction of the rule stage into matchRuleAndExecute is a verbatim move — same order, same early returns, same arguments apart from the extra selectorCache — so it is a refactor and not a silent behaviour change. The un-instrumented path really is one volatile read plus a null check with no allocation, because SelectorRuleMatchObservation.of is only reached after the null check. And the catch (RuntimeException ignored) wraps only the callback, so throwingObserverDoesNotAffectRequest is testing something real.
The concerns below are about the shape of the new extension point rather than about the observation values.
1. A second static registry, which the issue explicitly ruled out
Issue #8208 states: "Reuse #8203's optional observer identity/delivery conventions; do not introduce another observer registry, telemetry backend or matching engine."
SelectorRuleMatchObserverHolder is a new process-wide static volatile slot in shenyu-plugin-base. #8203's observers are threaded through the reactor context / constructor injection (AiRequestObservation and REACTOR_KEY), not reached through a global. This PR additionally ships the whole public surface — SelectorRuleMatchObserver, the holder, Decision, CacheResolution, SelectorRuleMatchObservation — with zero production registrant. No metrics wiring, no diagnostics wiring, no starter. AGENTS.md asks to reuse existing observability mechanisms and to avoid speculative options; a public SPI in shenyu-plugin-base that nothing registers is exactly that.
Two concrete consequences of the single-slot design:
register()silently overwrites, so a second consumer (say metrics, then diagnostics) orphans the first with no signal.MetricsReporter.registerat least deactivates the previous reporter; this does not.unregister()takes no token, so any component — or any test in the same JVM — can remove another consumer's observer.isInstrumented()has no caller in the tree.
If the intent is to land the adapter first and wire a consumer later, that is defensible, but then register should return a handle and unregister(handle) should be a no-op unless it is current, so the "exactly one observer" restriction cannot silently corrupt state.
2. Collapsed decisions hide boundaries the issue enumerated
The issue enumerates the branches separately. Decision merges them and relies on CacheResolution to disambiguate:
NO_SELECTORis used for "empty selector list", "negative selector cache hit" and (viaSELECTOR_UNMATCHED) the unmatched case is separate — but a consumer seesNO_SELECTOR+NEGATIVE_HITfor one andNO_SELECTOR+EVALUATED/NOT_CONSULTEDfor the other. Recoverable.NO_RULEis used for "empty rule list", "full-flow selector with no enabled rule" (:146) and "negative rule cache hit" (:161). For the first two, the observation is identical:NO_RULE,NOT_CONSULTED, no rule id. Those two are genuinely indistinguishable, and they have different operational meanings (a misconfigured selector vs. a rules table that only contains disabled rules).
If you keep one enum, add a distinct decision for the full-flow-without-enabled-rule case.
3. Plugin overrides are reported as "no match"
DividePlugin, WafPlugin, DubboPlugin, GrpcPlugin and WebSocketPlugin all override handleSelectorIfNull / handleRuleIfNull and take a fallback route there. The observation reports NO_SELECTOR / NO_RULE and then execution continues into a real route. For the operator this is meant to help ("did a selector match?"), that reads as "nothing matched" when in fact the plugin's own fallback did. Either mark those decisions as handled-by-override, or say in the javadoc that the observation stops at the shared matching stage and says nothing about plugin-specific fallbacks.
4. Three tests do not exercise production code
outerSkipProducesNoEventBecauseExecuteIsNeverReachedhand-emulates the chain instead of callingexecute.pluginsOutsideAbstractShenyuPluginProduceNoEventsassertsisEmpty()on a list it never populated.observationIsImmutableAndFreeOfRequestDatareflects over setters and field modifiers, which the compiler already guarantees.
None of these fail if the reporting is removed, so they inflate the coverage numbers without guarding the branches. Given the PR body says "Removing the reporting calls turns ten assertions red", please check that these three are not among them.
5. Missing coverage for the negative rule-cache hit
The tests warm the selector negative entry but not the rule one (:161). That branch is one of the enumerated boundaries and currently has no test.
6. "Must not block" is a comment, not a contract
SelectorRuleMatchObserver documents that the callback must not block, but dispatch is inline on the event loop (reportDecision runs on the Netty thread). A consumer that does synchronous I/O stalls every request on that thread, and the PR cannot prevent it — the sampling/queueing obligation is entirely the consumer's. That is an acceptable design, but the javadoc should say the callback runs on the event-loop thread so a consumer is not tempted to export inline.
Scope of this review: source reading of the diff and of AbstractShenyuPlugin, MatchDataCache and the overriding plugins. I did not run the module tests, so nothing here is a claim about CI.
…ision split, real-chain test
|
Thanks @wy471x for the careful review — all six points are addressed in 03131be:
|
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
Cross-file synthesis of PR #8291: all child partitions PASS, complete diff reviewed, but CI not passed. Lacking green CI, FINAL PASS impossible; no concrete defects found.
Evidence
- CI passed=false, complete=true, 9 checks in attachment
- all_parts_passed=true, complete_diff=true, 15 partitions PASS
- head_sha 03131be matched
Recommended next steps
- Wait for CI checks to pass (9 checks currently failed/pending)
- Re-run FINAL review after CI green to confirm PASS
- Verify observer registration in production config
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: a9bdb6b2-060e-4e61-afa8-9aa43291cd0b · Commit: 03131bec726366dbe907c2e16cb8550897211ef2
Review verdict: INCONCLUSIVE
FINAL scope: all partitions PASS, complete_diff true, but CI passed=false. Per FINAL rules, cannot issue PASS without CI green. No anchored HIGH/CRITICAL defects; uncertainty retained as INCONCLUSIVE.
Publication: COMMENT (INCONCLUSIVE_REVIEW)
…ing-image conflict)
|
The |
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
FINAL review of apache/shenyu PR #8291 at head f5e525a: all 16 partition reviews PASS with no HIGH/CRITICAL defects; complete diff supplied and CI green (64 checks). Added opt-in matching-decision observation is cross-file consistent.
Evidence
- 16 child partitions PASS, blocking_findings empty
- CI complete: 64 checks passed
- complete_diff true, head f5e525a
Recommended next steps
- Maintainer review and merge
- Consider docs for opt-in observation toggle
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 3371c9d0-bf87-450b-bbf2-d75991259b95 · Commit: f5e525acf237d105bad204a098f2f71a217c5756
Review verdict: PASS
FINAL synthesis: all_parts_passed true, complete_diff true, CI passed. Child reviews show consistent addition of SelectorRuleMatchObservation, holder, and tests without core logic alteration or defects.
Publication: APPROVE (EXPLICIT_PASS)
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
FINAL review of apache/shenyu PR #8291 at head f5e525a: all 16 partition reviews PASS with no HIGH/CRITICAL defects; complete diff supplied and CI green (64 checks). Added opt-in matching-decision observation is cross-file consistent.
Evidence
- 16 child partitions PASS, blocking_findings empty
- CI complete: 64 checks passed
- complete_diff true, head f5e525a
Recommended next steps
- Maintainer review and merge
- Consider docs for opt-in observation toggle
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 3371c9d0-bf87-450b-bbf2-d75991259b95 · Commit: f5e525acf237d105bad204a098f2f71a217c5756
Review verdict: PASS
FINAL synthesis: all_parts_passed true, complete_diff true, CI passed. Child reviews show consistent addition of SelectorRuleMatchObservation, holder, and tests without core logic alteration or defects.
Publication: COMMENT (MERGEABILITY_NOT_CONFIRMED)
Review details
Final triage: all partitions passed, complete diff and CI successful. The observability feature is coherent and test-covered. Recommend merge pending maintainer approval; no blocking changes requested.
Fixes #8208.
Problem
Operators cannot tell whether a plugin's selector and rule actually matched, which candidate won, or whether the per-path match cache supplied the result: the decision branches in
AbstractShenyuPluginare silent, and the only trace is an opt-in log line.Change
This PR adds a small optional matching-decision adapter at the real decision branches, following the opt-in static registration pattern already used by the diagnostics and metrics subsystems.
SelectorRuleMatchObservationis an immutable, bounded summary (plugin name, the decision reason, selected selector/rule ids, a synthetic-default-rule flag, and per-stageCacheResolutionof NOT_CONSULTED/POSITIVE_HIT/NEGATIVE_HIT/EVALUATED); it carries ids and enums only, never request paths, credentials, condition payloads or bodies.SelectorRuleMatchObserveris a single read-only callback whose contract forbids throwing, blocking, mutating the exchange or caches, and consuming request bodies — sampling, bounded queues and export backpressure remain the consumer's responsibility.SelectorRuleMatchObserverHolderregisters one observer; with none registered the matching path performs a single null read and allocates nothing. The branches ofAbstractShenyuPlugin.executereport exactly the decision just taken (missing/disabled plugin data, empty selector list, negative cache hit, unmatched selector, not-continued selector with the synthetic default rule, empty rule list, full-flow rule, negative rule cache hit, unmatched rule, selected); cached hits state their source only and never claim condition evaluation, and a SELECTED decision says nothing about later business success. Nothing re-runs any predicate and plugins outsideAbstractShenyuPluginget no fabricated events. The rule-stage tail ofexecuteis extracted verbatim intomatchRuleAndExecuteto keep methods within the checkstyle limit.Tests
SelectorRuleMatchObservationTestcovers every decision branch with the expected reason, ids, default-rule flag and cache resolutions (including two-request sequences that warm and then hit positive and negative cache entries), observer exceptions that leave the request untouched, the unregistered zero-event path, outer skip producing no event becauseexecuteis never reached, directShenyuPluginimplementations producing no events, and reflection checks that observations expose no setters and only final fields. Removing the reporting calls turns ten assertions red, confirming the tests guard the real branches.Verification
mvn -pl shenyu-plugin/shenyu-plugin-base,shenyu-plugin/shenyu-plugin-context-path,shenyu-plugin/shenyu-plugin-redirect -am clean test -DskipTests=falsepasses 376 + 9 + 4 tests with checkstyle clean. A 50M-decision holder fast-path probe measured 42-46 ms disabled versus 89-94 ms with a registered no-op consumer (~0.9 vs ~1.8 ns per decision, i.e. one volatile read versus a read plus one callback); this documents the opt-in cost only, and no end-to-end throughput benefit is claimed.