Repository navigation
OSPP feat(divide): support label-based canary routing and partition metrics - #7542
SouthwestAsiaFloat wants to merge 15 commits into
Conversation
…andle accordingly
Add deterministic partition decisions with extensible sticky-key sources. Preserve legacy gray routing and specify-domain behavior, keep retries within the selected pool, and validate mutually exclusive partition labels. Validation: 106 focused tests passed across admin, common, loadbalancer, divide, httpclient, and starter modules.
Record initial routing decisions and successful fallbacks through the existing exchange callback pattern, and record each external request outcome once. Reuse request latency with stable, canary and none partition labels, and use explicit millisecond buckets. Existing latency dashboards must account for the added label and changed bucket boundaries. Keep metric registration in MetricsReporter and support fractional-second decision observations in the Prometheus implementation. Document label cardinality, timing semantics and query migration. Validation: 173 selected tests passed, with Checkstyle and git diff --check.
Add an explicitly invoked integration test using ShenyuWebHandler, production plugins, real HTTP backends and a separate native Prometheus process. Check scraped request outcomes, fallback counts, decision and latency histograms, retry deduplication and bounded labels through PromQL. Add test-scoped plugin dependencies and document binary setup, reproduction, evidence output and cleanup. Admin configuration transport is outside scope. Validation: the end-to-end test passed with Prometheus 3.5.0 and 14 real requests; 120 Web and Metrics regression tests and Checkstyle also passed.
This reverts commit 0ac61bf.
|
this is the shenyu-dashboard modification accordingly apache/shenyu-dashboard#717 |
Aias00
left a comment
There was a problem hiding this comment.
Review: label-based canary routing and partition metrics (divide plugin)
A substantial (≈4200-line) feature adding label-based canary routing to the divide plugin plus partition metrics and admin-side discovery-upstream validation. It spans shenyu-common (CanaryConfig DTO + CanaryConfigValidator), shenyu-loadbalancer (legacy gray compat), shenyu-plugin-api (CanaryContext), shenyu-plugin-divide (CanaryDecisionService), shenyu-plugin-metrics (CanaryMetric), and shenyu-plugin-httpclient (retry body replay). Roughly 25 test classes accompany it — coverage is strong.
Correctness & safety points I verified
- ReDoS-safe validation:
CanaryConfigValidatorusescom.google.re2j.Pattern(linear-time RE2) forregexconditions — immune to catastrophic backtracking — and matches the gateway'sRegexPredicateJudgeengine. There2jdependency is added toshenyu-common/pom.xml. Invalid regex is rejected at parse time with a clear field error (canary.conditions[0].paramValue is invalid for regex). - Bounded metric cardinality: the
partitionlabel is derived fromcontext.getMetricPartition()and is limited tostable/canary/none— no URI, upstream URL, or request path in labels.MetricsReporter.recordTimegains a 3-arg overload (name, labelValues, duration) while the 2-arg form is kept for backward compatibility. This correctly avoids label explosion (consistent with the prior #7522 lessons). - Retry correctness:
DefaultRetryStrategyhandles canary fallback, andRequestBodyReplayRetryTestcovers WebClient request-body replay (the one-shot body semantics), so retries don't silently send empty bodies. - Testability:
DividePluginnow accepts an injectableCanaryDecisionService(defaultDefaultCanaryDecisionService), enabling unit tests without Spring.
CI: full matrix green — build + all shenyu-integrated-test-* + CodeQL pass.
Non-blocking suggestions
UpstreamCacheManager.updateUpstreamMetadatamutates cachedUpstreaminstances in place (the existing shenyu pattern). Acceptable, but field visibility under concurrent reads could be tightened (volatile / immutable snapshot) if you want stronger guarantees.findLegacyUpstreamListBySelectorIdis a compat shim for the legacy gray-exclusive pool. A short migration-timeline note in the docs would help operators understand when it can be removed.
Approving — the design is sound, validation is ReDoS-safe, and metrics labeling is correctly bounded.
|
Hi @SouthwestAsiaFloat, this PR currently has merge conflicts with the |
Preserve Canary validation, partition-aware failover, and metrics while incorporating upstream import deduplication and missing-upstream handling. Adapt the new config-import regression test to the Canary validator dependency.
Preserve upstream namespace-mapping coverage together with Canary upstream-label and health-check regression tests.
Warm the logging topic and wait for all subscribed queues before sending the request under test. Use isolated consumer groups and unique request markers, preserve the 30-second delivery assertion, and keep warm-up parameters separate from the target request.
|
@Aias00 Hi, this PR's CI/CD nows works well, I hope this pr and the corresponding pr at dashboard apache/shenyu-dashboard#717 could be handled first if permitted, |
Aias00
left a comment
There was a problem hiding this comment.
This is a well-documented 4k-line feature, and the design notes in the PR body (fallback rules, sticky-key semantics, metric names) are better than most. I cannot approve it as a single change, though, and the blockers below are structural rather than stylistic.
Blocking
- The branch is
DIRTY. It conflicts withmaster, and thepr_buildthat passed ran against an older base (3444fa06). Every line I reviewed may already be stale. Rebase first — reviewing a conflicted 4.1k-line diff is not a good use of anyone's time, least of all yours. - It collides with an already-approved PR. #7777 extracts
resendintoexecuteFailover(...)with aretryFilterparameter and makes it package-private; this PR edits the same method in place and addsFailoverExhaustedExceptioninside the oldonErrorResumeshape. One of the two has to be rebased onto the other, and the merged version needs to preserve both intents (failover partitioning and fixed-backoff reusability). Please state the plan. - It mixes five independently reviewable concerns across 54 files: upstream
labels+ admin validation/sync, canary decision logic, divide-plugin selection, retry partitioning, metrics, and an e2e case change. Even a maintainer with a free afternoon cannot reason about the blast radius in one pass. Suggested split, each shippable on its own:- (a)
Upstream.labels+ admin props validation + cache propagation (no routing change); - (b)
CanaryConfig+ validator +DividePluginpartitioning behind a disabled-by-default flag; - (c) retry/partition preservation (after #7777);
- (d) metrics — the new counters and the
partitionlabel on the existing histogram; - (e) docs.
- (a)
- Adding a
partitionlabel toshenyu_execute_latency_millisis a breaking observability change. Every existing dashboard, recording rule and alert that selects that metric withoutpartitionstill matches (fine), but anything doingsum by (le)/histogram_quantileacross partitions silently changes meaning, and users who alert on the metric by name will see new series appear. That needs an explicit release-notes entry and, ideally, the label addition in its own PR so it can be reverted independently.
Things I verified that look right
DivideRuleHandle.canaryis added toequals/hashCode/toString, so rule-handle cache invalidation sees config changes — easy to miss.SPECIFY_DOMAINstill takes precedence over canary selection, which keeps the explicit-pin escape hatch intact.Upstream.labelsis a separate map frommetadata, andUpstreamCacheManager.updateUpstreamMetadatarefreshes labels/metadata/gray on the cached instances rather than replacing them, which preserves health-check state. Marking the fieldsvolatileis consistent with that approach.LEGACY_GRAY_SELECTORSis cleaned inremoveByKeyas well as maintained insubmit, so the new side map does not leak per selector.
Questions
- Legacy gray behaviour changed. The PR body says "legacy routing may fall back to ordinary nodes when all gray nodes are unavailable", which is a behaviour change for existing users, not just an internal refactor. Please give a before/after table for the gray cases (gray configured and healthy / gray configured and all unhealthy / no gray) with the tests that pin each row.
re2jis now ashenyu-commondependency. The artifact is already managed in the root pom and used byshenyu-plugin-baseandshenyu-admin, so the marginal size cost is small — butshenyu-commonis pulled in by every client SDK, so widening it needs a sentence: what incommonneeds linear-time regex (presumably validating user-supplied canary condition patterns against ReDoS), and why that cannot live inshenyu-plugin-base.shenyu-e2e/.../logging/rocketmq/DividePluginCases.javais touched by a canary-routing PR. If that is an unrelated adaptation to a changed API, please note it; if it is needed for canary, it belongs in its own e2e case rather than modifying a logging test.
Review scope
I did not line-review CanaryConfig/CanaryConfigValidator/CanaryContext/DefaultCanaryDecisionService or the 411-line metrics test. Once the branch is rebased and split (or at least rebased, with a note that the split will follow), I will review the decision logic and the bucketing determinism in detail — those are the parts where a subtle bug produces wrong traffic splits rather than an exception.
Preserve canary partition routing through the shared failover helper and fixed backoff. Retain all Divide upstreams with the shared discovery handler, update cached labels by logical upstream identity, and accept the upstream RocketMQ test fix. Validation: 380 tests passed across 35 classes, with 2 existing skips. Checkstyle, Apache RAT, and git diff checks passed.
Motivation
Add rule-level canary routing to Divide. Upstream labels define Stable and Canary pools, while request conditions and a configured percentage determine the target partition. Metrics expose request outcomes, fallbacks, and latency by partition.
Changes
labelsin Props, including Admin validation, configuration synchronization, and gateway cache updates while preserving other Props fields.shenyu_canary_requests_total{selector,rule,partition,outcome}shenyu_canary_fallback_total{selector,rule,reason}shenyu_canary_decision_duration_seconds{selector,rule}partitionlabel to the existingshenyu_execute_latency_millishistogram. Record each external request outcome once, without counting internal retries as additional requests.Compatibility and behavior
graybehavior is preserved. This change does not add active failure detection when local health checks are disabled; legacy routing may fall back to ordinary nodes when all gray nodes are unavailable.Validation
./mvnw clean install -Dmaven.javadoc.skip=truevalidation required by the PR template has not yet been completed.Detailed validation report
Report filename:
CANARY_PR_VALIDATION_REPORT_20261011.mdThe report includes the proposal’s functional acceptance matrix, relevant regression tests after merging upstream, real Prometheus end-to-end validation, and performance measurements across three modes.The performance practice recorded might be inaccurate since there is a high load in my computer.
CANARY_PR_VALIDATION_REPORT_20261011.md
./mvnw clean install -Dmaven.javadoc.skip=true.