feat: Add cache on signature step - #148
Merged
Merged
Conversation
- Introduced envCatalog.ts to define environment variable specifications for server configuration. - Implemented helmValues.ts to render and validate Helm deployment values, including secret management. - Created passwordPolicy.ts to enforce password complexity rules consistent with server-side policies. - Enhanced Helm templates to support secret environment variables, ensuring secure handling of sensitive data. - Updated values.yaml to include a new secretEnv field for managing environment variables through Kubernetes Secrets. - Added tests for secret environment variable rendering and checksum annotations in Helm templates.
- Updated the licensedService function to eliminate the GeoResolver parameter. - Adjusted related test cases to reflect the removal of GeoResolver. - Simplified the Geo type to directly reference geoip.Location. - Removed RemoteIP from RequestFromRecord function and its usages. - Updated CheckInRecorder to remove RemoteIP handling in TouchDevice calls. - Enhanced ingest handler tests to remove unnecessary GeoResolver parameters. - Added configuration options for GeoIP header trust and MaxMind credentials in Helm values.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change replaces per-check-in goroutines with bounded workers, adds typed cache helpers, introduces provider and protocol caching with signature reuse, updates cache invalidation paths, and adds load-test and benchmark artifacts. ChangesRuntime check-in processing
Cache and protocol flow
Load-test evidence
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpoProtocolService
participant Cache
participant Repository
Client->>ExpoProtocolService: request update manifest
ExpoProtocolService->>Cache: read app and channel data
Cache->>Repository: query on cache miss
Repository-->>Cache: return repository data
Cache-->>ExpoProtocolService: return cached data
ExpoProtocolService-->>Client: return manifest and signature
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ypes; refactor signature handling
… refine dropped_iterations metric in load test
…es protocol - Documented configuration, methodology, and results of the load test conducted on 1 August 2026. - Included details on request rates, latency metrics, and server performance. - Provided instructions for reproducing the load test using k6.
# Conflicts: # README.md # internal/handlers/dashboard/rollouts_handler.go # internal/providers/expo/expo.go # internal/services/expo_protocol_service.go # internal/services/update_service.go # test/channel_mapping_cache_test.go # test/manifest_test.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
test/load/loadtest.js (1)
32-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the required environment variables in init.
The script reads four variables and never checks them. If
BASE_URLis unset, every request targetsundefined/manifestand the whole run is wasted. IfIOS_UPDATE_IDorANDROID_UPDATE_IDis unset, line 132 setsexpo-current-update-idtoundefined, so the up-to-date path is not exercised as the comment states. Fail fast in the init context instead.♻️ Proposed fail-fast check
const BASE = __ENV.BASE_URL; const APP_ID = __ENV.APP_ID; const UPDATE_IDS = { ios: __ENV.IOS_UPDATE_ID, android: __ENV.ANDROID_UPDATE_ID }; + +for (const [name, value] of Object.entries({ + BASE_URL: BASE, + APP_ID, + IOS_UPDATE_ID: UPDATE_IDS.ios, + ANDROID_UPDATE_ID: UPDATE_IDS.android, +})) { + if (!value) throw new Error(`Missing required environment variable: ${name}`); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/load/loadtest.js` around lines 32 - 34, Validate BASE_URL, APP_ID, and both UPDATE_IDS during the script’s init phase before any requests or scenario execution. Fail immediately with a clear error when any required environment variable is unset, while preserving the existing configuration values for valid runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ee/observe/checkins_queue_test.go`:
- Around line 29-61: Update TestCheckInQueueOverflowDropsAndReleasesClaim to
fill and block exactly checkInWorkerCount writes first, then wait until
store.calls.Load() confirms all workers are blocked before enqueueing the
remaining checkInQueueCapacity items. Only after the queue is filled should the
test record overflowing, preserving the existing assertions and release/drain
verification.
In `@ee/observe/checkins.go`:
- Around line 287-312: Extend the check-in claim lifetime beyond the current
10-second TTL so queued jobs remain claimable until the backlog is drained and
flush processing begins or retries. Update the claim handling around Record’s
job enqueue path and flush, preserving claim cleanup for dropped jobs while
preventing queued claims from expiring before processing.
In `@test/load/grafana-dashboard.json`:
- Around line 1079-1084: Add a Grafana template variable for the
server-under-test instance, then update the CPU, memory, and other node_exporter
queries in the affected panels to filter their metric selectors by that variable
(alongside the existing mode filter where applicable). Ensure all four panels
aggregate or display only the selected server instance rather than every scraped
node.
- Around line 168-193: Update the k6 duration panels using
k6_http_req_duration_p95/p99 to use milliseconds by changing their fieldConfig
defaults unit from "s" to "ms", and adjust the 0.25/1 thresholds to millisecond
values if they represent seconds. Also ensure p95 panels use a metric series
enabled by K6_PROMETHEUS_RW_TREND_STATS, replacing p95 expressions with an
available statistic where necessary.
In `@test/load/loadtest.js`:
- Around line 80-99: Update the push_storm documentation in
test/load/loadtest.js lines 80-99 to show Phase 3 ending at 16:50, and update
window_s in test/load/results/2026-08-01-summary.json lines 87-92 to [630, 1010]
so both records match the scenario’s actual duration.
In `@test/load/results/2026-08-01-summary.json`:
- Around line 113-120: Align the fleet statistics to one measurement window: in
test/load/results/2026-08-01-summary.json, recompute mean_ms over the
steady-state window or rename it to explicitly indicate whole-phase coverage;
update the corresponding fleet Mean cells in README.md at line 138 and
test/load/results/README.md at line 42 with the corrected value or matching
whole-phase label.
---
Nitpick comments:
In `@test/load/loadtest.js`:
- Around line 32-34: Validate BASE_URL, APP_ID, and both UPDATE_IDS during the
script’s init phase before any requests or scenario execution. Fail immediately
with a clear error when any required environment variable is unset, while
preserving the existing configuration values for valid runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f5144cc-5abb-47a8-b66d-af41d165f3c3
⛔ Files ignored due to path filters (1)
test/load/results/2026-08-01-timeseries.csvis excluded by!**/*.csv
📒 Files selected for processing (22)
README.mdee/observe/checkins.goee/observe/checkins_queue_test.gointernal/cache/cache.gointernal/cache/json.gointernal/handlers/dashboard/branches_handler.gointernal/handlers/dashboard/rollouts_handler.gointernal/providers/expo/expo.gointernal/providers/expo/expo_cache.gointernal/providers/expo/expo_cache_test.gointernal/services/expo_protocol_service.gointernal/services/protocol_cache.gointernal/services/protocol_cache_test.gointernal/services/update_service.gointernal/update/updates.gotest/cache_keys_test.gotest/channel_mapping_cache_test.gotest/load/grafana-dashboard.jsontest/load/loadtest.jstest/load/results/2026-08-01-summary.jsontest/load/results/README.mdtest/manifest_test.go
💤 Files with no reviewable changes (1)
- test/channel_mapping_cache_test.go
Comment on lines
+168
to
+193
| "targets": [ | ||
| { | ||
| "expr": "max(max_over_time(k6_http_req_duration_p95{testid=~\"$testid\"}[$__range]))" | ||
| } | ||
| ], | ||
| "fieldConfig": { | ||
| "defaults": { | ||
| "unit": "s", | ||
| "thresholds": { | ||
| "mode": "absolute", | ||
| "steps": [ | ||
| { | ||
| "color": "green", | ||
| "value": null | ||
| }, | ||
| { | ||
| "color": "yellow", | ||
| "value": 0.25 | ||
| }, | ||
| { | ||
| "color": "red", | ||
| "value": 1 | ||
| } | ||
| ] | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
k6 experimental-prometheus-rw output trend metric units milliseconds K6_PROMETHEUS_RW_TREND_STATS default
💡 Result:
When using the experimental-prometheus-rw output in k6, the K6_PROMETHEUS_RW_TREND_STATS environment variable defines which statistics are calculated and exported for k6 Trend metrics [1][2]. The default value for this setting is p(99) [1][3][2]. Regarding metric units and precision, k6 performs internal aggregation of trend metric data before sending it to the Prometheus remote write endpoint [1][3]. This is necessary because while k6 collects data points with nanosecond (ns) precision, Prometheus stores data with millisecond (ms) precision [1][3]. Aggregating these metrics also prevents the system from having to handle the vast amounts of raw data points that could be generated during a high-scale load test [1][3]. You can configure K6_PROMETHEUS_RW_TREND_STATS by providing a comma-separated list of statistics, such as count, sum, min, max, avg, med, or p(x) [1][4]. For example, setting K6_PROMETHEUS_RW_TREND_STATS=p(90),p(95),max will export three distinct Prometheus metrics for each k6 Trend metric, suffixed accordingly (e.g., k6_p90, k6p95, and k6*_max) [1][4].
Citations:
- 1: https://grafana.com/docs/k6/latest/results-output/real-time/prometheus-remote-write/?pg=building-a-synthetic-monitoring-solution-for-jaeger-with-grafana-k6&plcmt=in-text
- 2: https://github.com/grafana/k6/blob/1908adf8/internal/output/prometheusrw/remotewrite/config.go
- 3: https://grafana.com/docs/k6/latest/results-output/real-time/prometheus-remote-write/
- 4: https://grafana.com/docs/k6/next/results-output/real-time/grafana-cloud-prometheus/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dashboard panels around k6 duration targets and units =="
python3 - <<'PY'
import json
from pathlib import Path
p = Path('test/load/grafana-dashboard.json')
data = json.loads(p.read_text())
for panel in data.get('panels', []):
title = panel.get('title')
if 'latency' in title.lower() or 'p95' in title.lower() or 'p99' in title.lower() or 'mean' in title.lower():
print(f"\nPanel: {title}")
print(f" type: {panel.get('type')}")
print(f" targets: {json.dumps(panel.get('targets'), indent=2)}")
cfg = panel.get('fieldConfig', {})
defaults = cfg.get('defaults', {})
print(f" unit: {defaults.get('unit')}")
thresholds = defaults.get('thresholds', {})
if 'steps' in thresholds:
print(f" thresholds: {thresholds['steps']}")
PY
echo
echo "== k6-related references =="
rg -n "K6_PROMETHEUS_RW_TREND_STATS|experimental-prometheus-rw|k6_http_req_duration|prometheus|duration|thresholds" -S .Repository: mercuretechnologies/expo-open-ota
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files likely documenting k6 test/environment =="
git ls-files | rg -i '(^|/)(Dockerfile|docker-compose|Makefile|.*\.tf$|.*\.sh$|.*\.yaml$|.*\.yml$|.*\.toml$|.*\.env)$|^test/|load|k6' | head -200
echo
echo "== git diff stat =="
git diff --statRepository: mercuretechnologies/expo-open-ota
Length of output: 5801
Use milliseconds for the k6 duration panels.
k6 Prometheus remote-write exports trend metrics in milliseconds. These panels set "unit": "s" while reading k6_http_req_duration_p95/p99, so a 2.75 ms value renders as "2.75 s" and the 0.25/1 thresholds can pass incorrectly. Set the unit to ms, or divide the duration expressions by 1000. If K6_PROMETHEUS_RW_TREND_STATS only enables p(99), no p95 series are written and the p95 panels will never load values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/load/grafana-dashboard.json` around lines 168 - 193, Update the k6
duration panels using k6_http_req_duration_p95/p99 to use milliseconds by
changing their fieldConfig defaults unit from "s" to "ms", and adjust the 0.25/1
thresholds to millisecond values if they represent seconds. Also ensure p95
panels use a metric series enabled by K6_PROMETHEUS_RW_TREND_STATS, replacing
p95 expressions with an available statistic where necessary.
Source: Linters/SAST tools
Comment on lines
+80
to
+99
| // Phase 3 (10:30 -> 16:30) - rollout push storm on the 1M fleet. | ||
| // A push lands on every device; ~25% open within minutes, front-loaded. | ||
| // Rates below are APP OPENS per second; every open is an outdated device: | ||
| // full manifest + its asset requests. "Handling it" means zero errors, | ||
| // bounded queueing, and full drain once the wave decays. | ||
| // NOTE: needs k6 OSS or a paid plan (maxVUs > 100). Storm iterations | ||
| // span several requests, so in-flight VUs = opens/s x iteration duration. | ||
| push_storm: { | ||
| executor: 'ramping-arrival-rate', | ||
| exec: 'rolloutOpen', | ||
| startTime: '10m30s', | ||
| startRate: 20, timeUnit: '1s', | ||
| preAllocatedVUs: 300, maxVUs: 2500, | ||
| stages: [ | ||
| { target: 1000, duration: '30s' }, // the push lands | ||
| { target: 400, duration: '2m' }, // long tail of opens | ||
| { target: 100, duration: '2m' }, // decay | ||
| { target: 20, duration: '90s' }, // back to baseline; the queue must drain here | ||
| ], | ||
| }, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The push_storm duration is documented 20 seconds short in two places. The scenario starts at 10m30s and its stages total 380 s (30 s + 2 m + 2 m + 90 s), so it ends at 16:50, or 1010 s. Both documents use 16:30 / 990 s.
test/load/loadtest.js#L80-L99: change the header comment toPhase 3 (10:30 -> 16:50).test/load/results/2026-08-01-summary.json#L87-L92: changewindow_sto[630, 1010].
📍 Affects 2 files
test/load/loadtest.js#L80-L99(this comment)test/load/results/2026-08-01-summary.json#L87-L92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/load/loadtest.js` around lines 80 - 99, Update the push_storm
documentation in test/load/loadtest.js lines 80-99 to show Phase 3 ending at
16:50, and update window_s in test/load/results/2026-08-01-summary.json lines
87-92 to [630, 1010] so both records match the scenario’s actual duration.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request completes the rebranding of the project from "Expo Open OTA" to "xprem" across the codebase, documentation, and CI/CD workflows. It updates all references to the new name, ensures Docker images and Helm charts are published under both the new and old names for backward compatibility, and clarifies the open core policy and contact information.
Project rebranding and documentation updates:
CONTRIBUTING.mdandLICENSE.md, to refer to the project as "xprem" instead of "Expo Open OTA", and updated the contact email address. [1] [2] [3] [4]CI/CD workflow and artifact publishing:
xpremas the canonical image, while continuing to publish under the deprecatedexpo-open-otanames for compatibility. [1] [2]xpremas the primary chart, with a byte-identical chart published asexpo-open-otafor seamless upgrades; only the canonical chart is attached to releases. [1] [2]xpremas the canonical Docker image and Helm chart. [1] [2]Codebase changes:
Dockerfileto use the newxpremmodule path.Summary by CodeRabbit
Performance & Reliability
Documentation
Testing