Repository navigation
feat(ess): implement dynamic drift detection and warning propagation - #805
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces component drift detection for active experiments. It queries Spanner to check if live component generations have diverged from the experiment's locked snapshots and attaches a warning message to the experiment status when drift is detected. To support multiple queries, single-use Spanner clients in FetchExperiment and FetchExperimentStatus were replaced with read-only transactions. The review feedback focuses on database and code optimization: removing an unused column (HasPendingReconciliation) from the SQL query, replacing a hardcoded magic number (5) in the SQL query with a query parameter, and simplifying the component diff formatting logic by removing unreachable fallback code.
There was a problem hiding this comment.
Code Review
This pull request introduces a component drift detection mechanism for active experiments in the Spanner-backed repository, appending warning notices to the experiment status when snapshots diverge from live catalog generations. It also updates retrieval methods to evaluate drift within read-only transactions. The review feedback is highly constructive and identifies several improvement opportunities: transitioning from an INNER JOIN to a LEFT JOIN to properly detect hard-deleted components, updating the row parsing logic to handle nullable fields, removing an unused column from the consistency query, and capping the number of formatted drift details to prevent unbounded memory and string growth.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements component drift detection for active experiments, warning users when live component generations diverge from locked snapshots. It updates FetchExperiment and FetchExperimentStatus to evaluate drift within a read-only transaction and propagates these warnings. Feedback on the changes highlights a critical compilation issue due to a mismatched Spanner import path, as well as a potential formatting bug where an empty status message could result in a leading semicolon when appending the drift warning.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces component drift detection for active experiments. When fetching an experiment or its status, the system now evaluates whether the locked component generations in the experiment's snapshots have diverged from the live component catalog, appending a warning message if drift is detected. To support multiple queries within the same transaction, FetchExperiment and FetchExperimentStatus were updated to use read-only transactions. The review feedback suggests several code quality improvements: pre-allocating the diffs slice to prevent re-allocations, using row.ToStruct to reduce boilerplate when parsing Spanner rows, and refactoring the warning message attachment logic to enhance readability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces component drift detection for active experiments, checking if component snapshots have diverged from live catalog generations and attaching warning notices when drift is detected. The review feedback highlights a potential issue where evaluating experiment drift inside transaction-level helpers (FetchExperimentTransaction and fetchExperimentStatusTransaction) could cause unintended side-effects if called within read-write transactions. It is recommended to move the drift evaluation logic to the top-level read entry points (FetchExperiment and FetchExperimentStatus) to ensure the transaction helpers remain side-effect free.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces component drift detection for active experiments, checking if their component snapshots have diverged from the live catalog generations and appending warning notices to the experiment status. Feedback focuses on improving robustness and preserving status information: first, by using spanner.NullBool instead of a plain bool to safely handle potential NULL values from the consistency view; and second, by upgrading the severity and appending drift warnings to existing lower-severity status messages rather than overwriting them and discarding valuable information.
Implement dynamic drift evaluation in the Experiment State Server (ESS).
When fetching active experiment metadata via FetchExperiment and
FetchExperimentStatus, query the ExperimentConsistencyInfo Spanner view using
the BitReversedXid primary key. If live component catalog generations diverge
from immutable snapshots (or if a referenced component was soft-deleted),
construct and attach a structured drift warning notice with WARNING severity to
the experiment status message.
- Query ExperimentConsistencyInfo Spanner view by BitReversedXid to evaluate
snapshot consistency for active experiments without table scans.
- Query drifted component details comparing LockedGeneration vs ObservedGeneration
using JOIN@{JOIN_METHOD=APPLY_JOIN} to prevent unbounded hash joins.
- Filter out inactive/terminal experiments and pending reconciliations.
- Switch FetchExperiment and FetchExperimentStatus from client.Single() to
client.ReadOnlyTransaction() with deferred Close() to support multi-query
evaluation within a consistent snapshot.
- Propagate drift warning notices through ESS gRPC server read endpoints.
- Add emulator-backed Spanner storage tests and ESS server unit tests.
Fixes bug 548436674
TAG=agy
CONV=e8ed7a88-c276-4019-8fdf-996b07b49051
…ries - Remove unused HasPendingReconciliation column from selectExperimentConsistencySQL. - Pass componentStateDeleted as @deletedState query parameter in selectDriftedComponentsSQL. - Simplify formatComponentDiff by removing unreachable fallback branch. TAG=agy CONV=e8ed7a88-c276-4019-8fdf-996b07b49051
…t entries
- Update selectDriftedComponentsSQL to use LEFT JOIN@{JOIN_METHOD=APPLY_JOIN}
and select s.ComponentId to capture missing catalog components.
- Scan component fields into nullable Spanner types in formatComponentDiff and
explicitly format missing catalog components.
- Bound retrieved drifted components in fetchDriftDetails to maxDriftDetails (10).
Comment links:
- #805 (comment)
- #805 (comment)
- #805 (comment)
TAG=agy
CONV=e8ed7a88-c276-4019-8fdf-996b07b49051
… status - Use canonical import path "golang/cloud_google_com/go/spanner/v/v1/spanner" in driftdetection.go to match package conventions. - In attachDriftWarningMessage, check if GetUserFacingText() is empty before concatenating to avoid leading semicolon, and flatten with early returns. Comment links: - #805 (comment) - #805 (comment) TAG=agy CONV=e8ed7a88-c276-4019-8fdf-996b07b49051
…names in errors
- Update driftdetection_test.go to use the canonical Spanner package path ("golang/cloud_google_com/go/spanner/v/v1/spanner")
- Quote column names in formatComponentDiff and checkConsistency error wrapping to satisfy Effective Go formatting
TAG=agy
CONV=e8ed7a88-c276-4019-8fdf-996b07b49051
…y warning attachment - Pre-allocate diffs slice with capacity maxDriftDetails+1 in fetchDriftDetails - Define componentDiffData struct and use row.ToStruct in formatComponentDiff - Linearize early-return guard clauses and add clarifying comments in attachDriftWarningMessage TAG=agy CONV=e8ed7a88-c276-4019-8fdf-996b07b49051
- Move evaluateExperimentDrift to top-level FetchExperiment and FetchExperimentStatus - Keep FetchExperimentTransaction and fetchExperimentStatusTransaction side-effect free and safe for read-write transactions TAG=agy CONV=e8ed7a88-c276-4019-8fdf-996b07b49051
…ent dynamic drift Address Round 5 review feedback and synchronize documentation: - Use spanner.NullBool to safely decode IsExperimentConsistent from ExperimentConsistencyInfo, gracefully handling NULL aggregate outputs when experiments have no component snapshots. - Preserve existing lower-severity status messages by upgrading severity to WARNING and appending drift details rather than overwriting. - Add unit tests verifying no-snapshot consistency, lower-severity status merging, and higher-severity message preservation. - Add Section 6 to pkg/components/README.md detailing dynamic drift detection architecture, two-tier query strategy, and transaction isolation. - Add cmd/experiment_state_server/README.md outlining ESS responsibilities, storage layer interactions, and drift evaluation. TAG=agy CONV=e8ed7a88-c276-4019-8fdf-996b07b49051
…polling cache Protect against unmigrated database queries and hot-path polling churn: - Thread cfg.EnableComponents from ESS config into StorageConfig and experiment.RepositoryConfig. - In evaluateExperimentDrift, immediately bypass drift evaluation when enable_components is false, ensuring zero queries and zero overhead for non-component deployments. - Introduce a concurrent-safe in-memory driftCache on Repository with a 30-second TTL to avoid hitting Cloud Spanner on high-frequency UI / CLI status polling loops (e.g. GetExperimentStatus every 2s). - Add unit tests verifying that disabled components skip drift checks, and that active queries within the 30s TTL hit the memory cache. - Update pkg/components/README.md and cmd/experiment_state_server/README.md documenting feature flag gating and hot-path polling cache invariants. TAG=agy CONV=e8ed7a88-c276-4019-8fdf-996b07b49051
aaa746e to
c2c073e
Compare
- Change logging in evaluateExperimentDrift from logging.Info to logging.Warn so that operators configuring warning-level logging filters do not miss component drift events. Comment link: #805 (comment) TAG=agy CONV=e8ed7a88-c276-4019-8fdf-996b07b49051
Summary
Implements dynamic drift evaluation and warning propagation in the Experiment State Server (ESS). When fetching active experiment metadata or experiment status, ESS queries the
ExperimentConsistencyInfoSpanner view byBitReversedXidprimary key and attaches a structured warning notice (WARNINGseverity,XMANAGER_BACKENDsource) when live component catalog generations diverge from immutable experiment snapshots.Motivation & Context
In the XMC Components architecture, experiments lock immutable snapshots of component configurations and generations upon launch. If a shared component is upgraded or soft-deleted in the cluster catalog while an experiment is active, users and operators need immediate visibility into the configuration divergence without incurring full table scans, background cron loops, or blocking reads.
Key Changes
internal/storage/spanner/experiment/driftdetection.go,repository.go):enable_components(cfg.EnableComponentsviaStorageConfig). When disabled (default), drift checks immediately returnnil, guaranteeing zero Spanner queries and zero regression risk on unmigrated databases.driftCachewith a 30-second TTL (driftCacheTTL = 30 * time.Second) onRepositoryto eliminate Spanner view query churn on high-frequency UI console and CLI status polling loops (GetExperimentStatusevery 2s).evaluateExperimentDriftto inspect active experiment snapshot consistency via theExperimentConsistencyInfoview using primary keyBitReversedXid.spanner.NullBoolto safely decodeIsExperimentConsistent, gracefully handling NULL aggregate outputs when experiments have no component snapshots.fetchDriftDetailsusing an explicit Spanner optimizer hintJOIN@{JOIN_METHOD=APPLY_JOIN}andLEFT JOINonExperimentComponentSnapshotsandComponentsto capture both diverged/soft-deleted and missing catalog components without table scans.maxDriftDetails = 10to prevent memory spikes or oversized status messages.diffsslice capacity to eliminate runtime slice re-allocations.@deletedStateand eliminated unused SELECT columns (HasPendingReconciliation)."golang/cloud_google_com/go/spanner/v/v1/spanner".row.ToStruct(&componentDiffData)and nullable Spanner types informatComponentDiff.attachDriftWarningMessageto safely set or merge drift warnings intoExperimentStatus.structured_status_message: upgrades lower-severity messages (INFO,UNSPECIFIED) toWARNINGand appends drift text without overwriting original content, while preserving higher-severity messages (ERROR,FATAL) untouched.fetchexperiment.go,fetchexperimentstatus.go):client.Single()to scopedclient.ReadOnlyTransaction()withdefer txn.Close()to support multi-query drift evaluation within a strongly consistent snapshot.FetchExperimentandFetchExperimentStatus), keeping transaction helpers (FetchExperimentTransactionandfetchExperimentStatusTransaction) completely side-effect free and safe for read-write transactions.cmd/experiment_state_server/server/):cfg.EnableComponentsintoProdServerstorage initialization inprod.go.GetExperimentandGetExperimentStatus.driftdetection_test.goverifying consistent snapshots, diverged generations, soft deletions, terminal state filtering, pending reconciliations, no-snapshot edge cases, lower-severity message merging, higher-severity message preservation, disabled components bypass, and 30s TTL cache hits.pkg/components/README.md,cmd/experiment_state_server/README.md):pkg/components/README.mddetailing dynamic drift detection architecture, two-tier query strategy, transaction isolation, feature flag gating, and hot-path polling cache.cmd/experiment_state_server/README.mdoutlining ESS service responsibilities, storage layer interactions, and drift evaluation.Assumptions
.protocontracts with new dedicated fields (which would alter the public API contract), drift warning notices are dynamically injected intoExperimentStatus.structured_status_messagewithSeverity = WARNINGandSource = XMANAGER_BACKEND— Assumed to preserve existing proto schema backward compatibility without requiring client protobuf regeneration — If wrong, dedicated protobuf fieldsis_experiment_consistentanddrift_warningswould need to be added toapi/protos/experiment/*.proto.ExperimentConsistencyInfoSpanner view filtersWHERE e.ActiveState = 1, and terminal/inactive experiments skip drift detection — Assumed intentional per components system invariants because drift warnings are actionable only for active experiments — If wrong, historical drift would need to be computed against snapshot audit logs.Verification & Testing
bazel test //internal/storage/spanner/experiment:experiment_test(PASSED in 31.5s against Spanner emulator)bazel test //cmd/experiment_state_server/...(PASSED in 43.4s)gofmtverified clean.buildifierverified clean on all touched BUILD files../.githooks/pre-commitpassed all checks (buildifier, gofmt, golangci-lint, go test determinism, addlicense).APPLY_JOINquery hint)tools/pr_review/verify_pr_comment_resolution.py.Related Issues / Bugs
Fixes bug 548436674
TAG=agy
CONV=e8ed7a88-c276-4019-8fdf-996b07b49051