Skip to content

OSPP feat(divide): support label-based canary routing and partition metrics - #7542

Closed
SouthwestAsiaFloat wants to merge 15 commits into
apache:masterfrom
SouthwestAsiaFloat:ospp2662f0084
Closed

SouthwestAsiaFloat wants to merge 15 commits into
apache:masterfrom
SouthwestAsiaFloat:ospp2662f0084

Conversation

@SouthwestAsiaFloat

Copy link
Copy Markdown
Contributor

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

  • Support upstream labels in Props, including Admin validation, configuration synchronization, and gateway cache updates while preserving other Props fields.
  • Add Canary configuration to Divide rules, including request conditions, traffic percentage, sticky key, partition labels, and empty-pool policies.
  • Reuse existing condition matching and parameter extraction. Deterministic bucketing based on the rule ID and sticky key keeps partition selection consistent for the same configuration and key.
  • Reuse existing load balancing and circuit-breaker filtering within the selected partition. HTTP retries retain that partition and use refreshed upstream candidates.
  • Extend the existing Metrics plugin and Prometheus reporter with:
    • shenyu_canary_requests_total{selector,rule,partition,outcome}
    • shenyu_canary_fallback_total{selector,rule,reason}
    • shenyu_canary_decision_duration_seconds{selector,rule}
  • Add a partition label to the existing shenyu_execute_latency_millis histogram. Record each external request outcome once, without counting internal retries as additional requests.
  • Add configuration, routing, retry, and metrics tests, along with documentation covering metric semantics and PromQL examples.

Compatibility and behavior

  • Rules without Canary configuration retain their existing routing behavior. Disabling an existing Canary configuration routes requests to Stable.
  • Requests enter Stable when conditions do not match, the percentage is 0, or a required sticky key is missing from the request.
  • An empty Canary candidate pool falls back to Stable or rejects the request according to configuration. An empty Stable candidate pool rejects the request.
  • Existing health-check and legacy gray behavior 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.
  • Metrics do not include sticky keys, user IDs, request IDs, or sensitive Header/Cookie values.
  • Request latency retains the existing timing logic. The additional partition label and updated histogram buckets require reviewing existing dashboard queries. Quantiles should not combine incompatible old and new bucket layouts during rolling upgrades.

Validation

  • After merging upstream, 505 relevant tests across 43 test classes passed.
  • Real Prometheus end-to-end validation passed, covering scraping, PromQL, partition request counts, error rates, latency, fallbacks, rejections, and retry deduplication.
  • Completed the performance matrix for 10, 100, and 1,000 logical upstream nodes across the original baseline, the current version without Canary configuration, and the current version with Canary enabled, with three rounds per combination.
  • Host load varied substantially during performance testing. These results are measurement records, not evidence of meeting performance targets or establishing a performance regression.
  • The full-repository ./mvnw clean install -Dmaven.javadoc.skip=true validation required by the PR template has not yet been completed.

Detailed validation report

Report filename: CANARY_PR_VALIDATION_REPORT_20261011.md

The 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

  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

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.
@SouthwestAsiaFloat

Copy link
Copy Markdown
Contributor Author

this is the shenyu-dashboard modification accordingly apache/shenyu-dashboard#717

Aias00
Aias00 previously approved these changes Oct 11, 2026

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: CanaryConfigValidator uses com.google.re2j.Pattern (linear-time RE2) for regex conditions — immune to catastrophic backtracking — and matches the gateway's RegexPredicateJudge engine. The re2j dependency is added to shenyu-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 partition label is derived from context.getMetricPartition() and is limited to stable / canary / none — no URI, upstream URL, or request path in labels. MetricsReporter.recordTime gains 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: DefaultRetryStrategy handles canary fallback, and RequestBodyReplayRetryTest covers WebClient request-body replay (the one-shot body semantics), so retries don't silently send empty bodies.
  • Testability: DividePlugin now accepts an injectable CanaryDecisionService (default DefaultCanaryDecisionService), enabling unit tests without Spring.

CI: full matrix green — build + all shenyu-integrated-test-* + CodeQL pass.

Non-blocking suggestions

  • UpstreamCacheManager.updateUpstreamMetadata mutates cached Upstream instances in place (the existing shenyu pattern). Acceptable, but field visibility under concurrent reads could be tightened (volatile / immutable snapshot) if you want stronger guarantees.
  • findLegacyUpstreamListBySelectorId is 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.

@Aias00

Aias00 commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Hi @SouthwestAsiaFloat, this PR currently has merge conflicts with the master base branch, so CI cannot validate it against the latest code. Please rebase/merge the latest master and resolve the conflicts, then push the updated branch. Once the conflicts are cleared, CI will re-run automatically and we can continue the review. Thanks!

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.
@SouthwestAsiaFloat

Copy link
Copy Markdown
Contributor Author

@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 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. The branch is DIRTY. It conflicts with master, and the pr_build that 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.
  2. It collides with an already-approved PR. #7777 extracts resend into executeFailover(...) with a retryFilter parameter and makes it package-private; this PR edits the same method in place and adds FailoverExhaustedException inside the old onErrorResume shape. 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.
  3. 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 + DividePlugin partitioning behind a disabled-by-default flag;
    • (c) retry/partition preservation (after #7777);
    • (d) metrics — the new counters and the partition label on the existing histogram;
    • (e) docs.
  4. Adding a partition label to shenyu_execute_latency_millis is a breaking observability change. Every existing dashboard, recording rule and alert that selects that metric without partition still matches (fine), but anything doing sum by (le)/histogram_quantile across 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.canary is added to equals/hashCode/toString, so rule-handle cache invalidation sees config changes — easy to miss.
  • SPECIFY_DOMAIN still takes precedence over canary selection, which keeps the explicit-pin escape hatch intact.
  • Upstream.labels is a separate map from metadata, and UpstreamCacheManager.updateUpstreamMetadata refreshes labels/metadata/gray on the cached instances rather than replacing them, which preserves health-check state. Marking the fields volatile is consistent with that approach.
  • LEGACY_GRAY_SELECTORS is cleaned in removeByKey as well as maintained in submit, so the new side map does not leak per selector.

Questions

  1. 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.
  2. re2j is now a shenyu-common dependency. The artifact is already managed in the root pom and used by shenyu-plugin-base and shenyu-admin, so the marginal size cost is small — but shenyu-common is pulled in by every client SDK, so widening it needs a sentence: what in common needs linear-time regex (presumably validating user-supplied canary condition patterns against ReDoS), and why that cannot live in shenyu-plugin-base.
  3. shenyu-e2e/.../logging/rocketmq/DividePluginCases.java is 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants