feat(analytics): add W64 runtime history and SLO APIs - #245
ammarheidari wants to merge 1 commit into
Conversation
|
@codex review Please review exact W64 backend head |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review Please review current W64 runtime source head |
99d6528 to
a6a47f5
Compare
|
@codex review Please review canonical exact W64 backend head |
Refs #213 Signed-off-by: Ammar Heidari <ammar@arad-itc.org>
9ca46f9 to
e55f28e
Compare
|
@codex review Review final exact W64 backend head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e55f28ee98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| builder.Services.AddHostedService< | ||
| ConsumerLagHistorySamplingHostedService>(); |
There was a problem hiding this comment.
When historical metrics run in HighAvailability mode, every API replica registers this hosted service, so each replica independently issues up to 500 lag reads per cluster and writes another set of samples every interval. The per-process semaphore therefore does not provide a deployment-wide bound, and the slightly different observation timestamps also make these duplicate samples distinct rows that can bias later SLO calculations. Gate sampling with a shared lease/fence, as the historical maintenance worker does, or otherwise nominate a single sampler.
Useful? React with 👍 / 👎.
| var state = | ||
| points.Length == 0 | ||
| ? OperationalTrendState.Unknown | ||
| : points.Any(point => | ||
| point.State is not | ||
| OperationalEvidenceState.Available) | ||
| ? OperationalTrendState.Partial | ||
| : OperationalTrendState.Available; |
There was a problem hiding this comment.
When a trend contains more than MaxPoints, the store returns raw.Truncated = true, but this state calculation can still label the trend Available if every returned point is available. EvaluateSloAsync then computes compliance from only that truncated prefix and also reports the SLO as available, while OperationalSloResult carries no truncation field to reveal the omitted evidence. Include truncation in the partial-state decision so bounded queries cannot produce apparently complete SLO results.
Useful? React with 👍 / 👎.
| var state = | ||
| usable.Length != | ||
| trend.Points.Count || | ||
| trend.State == | ||
| OperationalTrendState.Partial | ||
| ? OperationalEvidenceState.Partial | ||
| : OperationalEvidenceState.Available; |
There was a problem hiding this comment.
If sampling succeeds only once during a requested hour—for example because Kafka reads failed for the other intervals—the returned history contains one available point and this logic reports the SLO as fully Available, because it compares usable points only with points that happened to be returned. No check establishes coverage of the requested fromUtc/toUtc window or detects missing sampler intervals, so a sparse success can yield misleading 0% or 100% compliance. Use the coverage timestamps and expected sampling cadence to mark incomplete windows partial or unknown.
Useful? React with 👍 / 👎.
| var selected = | ||
| groups.Value | ||
| .OrderBy(group => | ||
| group.GroupId, | ||
| StringComparer.Ordinal) | ||
| .Take( | ||
| _policy | ||
| .MaxGroupsPerCluster) | ||
| .ToArray(); |
There was a problem hiding this comment.
For clusters with more than MaxGroupsPerCluster groups, sorting and taking the same prefix on every cycle permanently excludes every group after the cap. Those groups can still be queried through the new trend/SLO endpoints but will never acquire history, even across arbitrarily many sampling intervals. Keep the per-cycle bound while rotating or cursoring the selected subset so all groups eventually receive samples.
Useful? React with 👍 / 👎.
| var exact = | ||
| total <= | ||
| LargestExactlyRepresentableInteger; |
There was a problem hiding this comment.
When total is 9_007_199_254_740_993, comparison with this double constant first converts the long to double, rounding it down to 9_007_199_254_740_992; exact consequently becomes true. The same conversion rounds the stored gauge value, so the sampler records an inexact value as Stable, unlike the live path which correctly uses a long bound. Make the precision limit a long to preserve the intended comparison.
Useful? React with 👍 / 👎.
| var raw = | ||
| await _history.QueryAsync( | ||
| new HistoricalMetricQuery( | ||
| metricName, | ||
| query.Resource.ClusterId, | ||
| OperationalMetricHistoryNames | ||
| .ResourceKind( | ||
| query.Resource.Kind), | ||
| query.Resource.ResourceId, | ||
| query.FromUtc, | ||
| query.ToUtc, | ||
| MaxSeries: 1, | ||
| query.MaxPoints), | ||
| cancellationToken) | ||
| .ConfigureAwait(false); |
There was a problem hiding this comment.
When a configured history store times out or becomes unavailable, this call propagates the exception; the trend and SLO endpoints catch only ArgumentException, so clients receive a generic 500 instead of the explicit unavailable evidence returned when no provider is configured. Catch provider timeout/unavailability failures here, while preserving caller cancellation, and return an Unavailable trend so configured-but-failing providers obey the same evidence-truth contract.
Useful? React with 👍 / 👎.
| var state = | ||
| point.State switch | ||
| { | ||
| "Partial" => | ||
| OperationalEvidenceState.Partial, | ||
| "Unknown" => | ||
| OperationalEvidenceState.Unknown, | ||
| _ => | ||
| OperationalEvidenceState.Available, | ||
| }; |
There was a problem hiding this comment.
Any provider state other than the exact strings Partial and Unknown falls through to Available. A valid stored point marked Stale (or another non-stable provider state) is therefore presented as available and is included in SLO compliance, changing provider truth rather than conservatively preserving it. Map Stable explicitly to available and map recognized stale/unavailable states appropriately, treating unknown state strings as partial or unknown rather than available.
Useful? React with 👍 / 👎.
2 participants
Authority
W64 #213 is ACTIVE. W63 historical metrics is complete and protected-main verified.
Backend completion slice
Unavailablewhen no real metrics provider exists;ConsumerReadauthorization;Safety
Exact head:
e55f28ee985238ac0aac6f4f2b53f69752ceb74d.Refs #213.
Canonical runtime reconciliation
fd9dc052cfc0e31b6b56d06a7147f462ce2f709a;a6a47f53214eae49ca7a5c4ffb82c4eb693d0158;Final exact-head replay
fd9dc052cfc0e31b6b56d06a7147f462ce2f709a;e55f28ee985238ac0aac6f4f2b53f69752ceb74d;