feat(function-autoscaler): add LLM gateway scaling - #727
Open
borao wants to merge 1 commit into
Open
Conversation
Signed-off-by: Bora Oztekin <boztekin@nvidia.com>
📝 WalkthroughWalkthroughThe autoscaler now discovers LLM Gateway metrics, selects metric sources through a shared routing cache, chooses gateway targets, and calculates source-specific scaling inputs and desired instance changes. ChangesLLM Gateway autoscaling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant Autoscaler as run_autoscaling_logic_p0
participant Gathering as scaling-input gathering
participant Cache as MetricRoutingCache
participant Gateway as gateway target selection
Server->>Cache: create shared routing cache
Server->>Autoscaler: pass routing cache
Autoscaler->>Gathering: gather scaling inputs
Gathering->>Cache: read or store routing decision
Gathering->>Gateway: select gateway version and target
Gateway-->>Gathering: return target and instance state
Gathering-->>Autoscaler: return inputs and selected target
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
🛡️ CodeQL Analysis🚨 Found 2 issue(s) Severity Breakdown:
📋 Top Issues🔗 View full details in Security tab 🕐 Last updated: 2026-08-08 01:04:34 UTC | Commit: 772185c |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs (2)
57-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a TTL to
gateway_targets, and consider moving this cache into the scaling module.Two points:
sourcesexpires after one hour.gateway_targetshas only a capacity bound, so a version pin can live for the whole process lifetime.select_gateway_targetdoes drop a stale pin when the pinned version disappears fromnvcf_function_infoor becomes idle while another version is active. A pin can still persist for days when all versions stay idle. A TTL makes the stickiness window explicit and bounded.- The coding guidelines place policy caches and stickiness behavior in the scaling module.
gateway_targetsis a stickiness cache, andselect_gateway_targetis stickiness logic.♻️ Proposed TTL change
pub fn new_metric_routing_cache() -> MetricRoutingCache { let ttl = StdDuration::from_secs(60 * 60); MetricRoutingCache { sources: Cache::builder().time_to_live(ttl).build(), - gateway_targets: Cache::new(10_000), + gateway_targets: Cache::builder() + .max_capacity(10_000) + .time_to_live(ttl) + .build(), } }As per coding guidelines: "Keep scaling logic, policy clients and caches, thresholds, and stickiness behavior within the scaling module."
🤖 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 `@src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs` around lines 57 - 69, Update new_metric_routing_cache to configure gateway_targets with the same one-hour time-to-live as sources while retaining its existing capacity bound. Move MetricRoutingCache and the related select_gateway_target stickiness logic into the scaling module, preserving their current behavior and interfaces.Source: Coding guidelines
419-438: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the request counter for activity, and guard a zero-minute lookback.
Two points:
- This query measures activity with
increase(..._duration_seconds_sum[...]). It reports idle when the gateway records a zero duration for every request in the window.llm_api_gateway_http_requests_totalis already used byllm_gateway_metrics_present, and it is the direct activity signal.recently_invokeddrives scale-to-zero, so a false negative terminates a live deployment.lookback_minutescomes fromscale_to_zero_idle_timeout.as_secs() as i64 / 60at Line 613. If the timeout is configured below 60 seconds, the value is0and the selector becomes[0m], which PromQL rejects. The error then propagates through?and fails the whole gather for the function.♻️ Proposed change
let end_time = Utc::now(); + let lookback_minutes = lookback_minutes.max(1); let query = format!( - r#"sum by(function_id) (increase(llm_api_gateway_http_request_duration_seconds_sum{{function_id="{}"}}[{}m])) > 0"#, + r#"sum by(function_id) (increase(llm_api_gateway_http_requests_total{{function_id="{}"}}[{}m])) > 0"#, function_id, lookback_minutes );🤖 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 `@src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs` around lines 419 - 438, Update llm_gateway_recently_invoked to query llm_api_gateway_http_requests_total instead of the duration sum, preserving the existing activity check for scale-to-zero decisions. Normalize a zero-minute lookback to a valid positive PromQL range before constructing the query, so sub-minute idle timeouts do not produce an invalid [0m] selector or propagate a query error.
🤖 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
`@src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs`:
- Around line 343-351: The gateway scaling flow must prevent idle versions from
retaining surplus instances and must not reduce the selected version to zero
when shared capacity is redistributed. Update gateway_target_desired_instances
and the surrounding Line 812 per-version handling so non-selected versions
receive scale-down requests, while the selected target retains at least the
count required by the shared desired-total decision; revise the test covering
the current saturating-subtraction floor accordingly.
- Around line 353-417: Scope all gateway metrics to the current environment:
update get_gateway_target in
src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs
(lines 353-417) to accept env and ignore_env and apply the appropriate matcher
to both nvcf_function_instances_current and nvcf_function_info; align the
LlmGateway numerator and denominator in the same file (lines 115-141), or
document at both sites if either metric lacks an environment label. Update
discovery in
src/control-plane-services/function-autoscaler/crates/server/src/work/discovery.rs
(lines 188-199) to reuse get_timeseries_db_query’s aws_env matcher while
honoring ignore_env, and adjust the assertion at line 995 accordingly.
- Around line 115-141: Update the MetricSource::LlmGateway query in the metric
query construction to divide the in-flight request rate by both current
instances and nvcf_function_concurrency, matching the ControlPlane utilization
formula. Preserve the existing function_id grouping and zero-safe denominator
behavior so the result remains a percentage calibrated for decide_scaling.
- Around line 502-637: The TimeseriesDb helper errors are not currently recorded
in the request span. Update the existing tracing instrumentation on the shared
request execution function reached via query_range to include the returned error
field using err, preserving the existing helper behavior and span coverage.
---
Nitpick comments:
In
`@src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs`:
- Around line 57-69: Update new_metric_routing_cache to configure
gateway_targets with the same one-hour time-to-live as sources while retaining
its existing capacity bound. Move MetricRoutingCache and the related
select_gateway_target stickiness logic into the scaling module, preserving their
current behavior and interfaces.
- Around line 419-438: Update llm_gateway_recently_invoked to query
llm_api_gateway_http_requests_total instead of the duration sum, preserving the
existing activity check for scale-to-zero decisions. Normalize a zero-minute
lookback to a valid positive PromQL range before constructing the query, so
sub-minute idle timeouts do not produce an invalid [0m] selector or propagate a
query error.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 790bb3fa-c58a-4336-af88-301c8ccb43b8
📒 Files selected for processing (4)
src/control-plane-services/function-autoscaler/crates/server/src/scaling/mod.rssrc/control-plane-services/function-autoscaler/crates/server/src/server.rssrc/control-plane-services/function-autoscaler/crates/server/src/work/discovery.rssrc/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs
Comment on lines
+115
to
141
| let query = match metric_source { | ||
| MetricSource::ControlPlane => format!( | ||
| r#"100 * sum by(function_id, function_version_id, nca_id) (rate(function_request_latency_sum{{function_id="{id}", function_version_id="{v_id}"{env}}}[2m])) / | ||
| (avg by(function_id, function_version_id, nca_id) (nvcf_function_instances_current{{function_id="{id}", function_version_id="{v_id}"{env}}}) * avg by(function_id, function_version_id, nca_id) (nvcf_function_concurrency{{function_id="{id}", function_version_id="{v_id}"{env}}})) or vector(0)"#, | ||
| id = function_id, | ||
| v_id = function_version_id, | ||
| env = env_suffix | ||
| ) | ||
| } else { | ||
| format!( | ||
| ), | ||
| MetricSource::WorkerThreads => format!( | ||
| r#"((sum by(function_id, function_version_id, nca_id) (increase(nvcf_worker_service_worker_thread_busy_seconds_total{{function_id="{id}", function_version_id="{v_id}"{env}}}[{window}s]))) / {window} * 100) / | ||
| (sum by(function_id, function_version_id, nca_id) (nvcf_worker_service_worker_thread_count_total{{function_id="{id}", function_version_id="{v_id}"{env}}}))"#, | ||
| id = function_id, | ||
| v_id = function_version_id, | ||
| env = env_suffix, | ||
| window = utilization_window_seconds | ||
| ) | ||
| ), | ||
| MetricSource::LlmGateway => { | ||
| // Little's Law: average duration * request rate collapses to the | ||
| // per-second rate of the duration sum. | ||
| format!( | ||
| r#"100 * sum by(function_id) (rate(llm_api_gateway_http_request_duration_seconds_sum{{function_id="{id}"}}[2m])) / | ||
| clamp_min(sum by(function_id) (nvcf_function_instances_current{{function_id="{id}"{env}}}), 1)"#, | ||
| id = function_id, | ||
| env = env_suffix, | ||
| ) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The gateway utilization formula ignores per-instance concurrency.
The ControlPlane arm divides by instances * nvcf_function_concurrency. The LlmGateway arm divides by instances only. rate(duration_seconds_sum[2m]) is average in-flight request concurrency. Dividing it by the instance count gives requests in flight per instance, not a utilization ratio.
For a function with nvcf_function_concurrency = 4, one fully busy instance reports 400% instead of 100%. decide_scaling then applies the scale-up factor against a threshold that is calibrated for a percentage, so the autoscaler over-provisions gateway functions by a factor equal to the configured concurrency.
Divide by concurrency as the control-plane arm does, or document why gateway functions are always concurrency 1.
🐛 Proposed fix
format!(
r#"100 * sum by(function_id) (rate(llm_api_gateway_http_request_duration_seconds_sum{{function_id="{id}"}}[2m])) /
- clamp_min(sum by(function_id) (nvcf_function_instances_current{{function_id="{id}"{env}}}), 1)"#,
+ clamp_min(
+ sum by(function_id) (nvcf_function_instances_current{{function_id="{id}"{env}}})
+ * on(function_id) group_left() max by(function_id) (nvcf_function_concurrency{{function_id="{id}"{env}}}),
+ 1
+ )"#,
id = function_id,
env = env_suffix,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let query = match metric_source { | |
| MetricSource::ControlPlane => format!( | |
| r#"100 * sum by(function_id, function_version_id, nca_id) (rate(function_request_latency_sum{{function_id="{id}", function_version_id="{v_id}"{env}}}[2m])) / | |
| (avg by(function_id, function_version_id, nca_id) (nvcf_function_instances_current{{function_id="{id}", function_version_id="{v_id}"{env}}}) * avg by(function_id, function_version_id, nca_id) (nvcf_function_concurrency{{function_id="{id}", function_version_id="{v_id}"{env}}})) or vector(0)"#, | |
| id = function_id, | |
| v_id = function_version_id, | |
| env = env_suffix | |
| ) | |
| } else { | |
| format!( | |
| ), | |
| MetricSource::WorkerThreads => format!( | |
| r#"((sum by(function_id, function_version_id, nca_id) (increase(nvcf_worker_service_worker_thread_busy_seconds_total{{function_id="{id}", function_version_id="{v_id}"{env}}}[{window}s]))) / {window} * 100) / | |
| (sum by(function_id, function_version_id, nca_id) (nvcf_worker_service_worker_thread_count_total{{function_id="{id}", function_version_id="{v_id}"{env}}}))"#, | |
| id = function_id, | |
| v_id = function_version_id, | |
| env = env_suffix, | |
| window = utilization_window_seconds | |
| ) | |
| ), | |
| MetricSource::LlmGateway => { | |
| // Little's Law: average duration * request rate collapses to the | |
| // per-second rate of the duration sum. | |
| format!( | |
| r#"100 * sum by(function_id) (rate(llm_api_gateway_http_request_duration_seconds_sum{{function_id="{id}"}}[2m])) / | |
| clamp_min(sum by(function_id) (nvcf_function_instances_current{{function_id="{id}"{env}}}), 1)"#, | |
| id = function_id, | |
| env = env_suffix, | |
| ) | |
| } | |
| }; | |
| let query = match metric_source { | |
| MetricSource::ControlPlane => format!( | |
| r#"100 * sum by(function_id, function_version_id, nca_id) (rate(function_request_latency_sum{{function_id="{id}", function_version_id="{v_id}"{env}}}[2m])) / | |
| (avg by(function_id, function_version_id, nca_id) (nvcf_function_instances_current{{function_id="{id}", function_version_id="{v_id}"{env}}}) * avg by(function_id, function_version_id, nca_id) (nvcf_function_concurrency{{function_id="{id}", function_version_id="{v_id}"{env}}})) or vector(0)"#, | |
| id = function_id, | |
| v_id = function_version_id, | |
| env = env_suffix | |
| ), | |
| MetricSource::WorkerThreads => format!( | |
| r#"((sum by(function_id, function_version_id, nca_id) (increase(nvcf_worker_service_worker_thread_busy_seconds_total{{function_id="{id}", function_version_id="{v_id}"{env}}}[{window}s]))) / {window} * 100) / | |
| (sum by(function_id, function_version_id, nca_id) (nvcf_worker_service_worker_thread_count_total{{function_id="{id}", function_version_id="{v_id}"{env}}}))"#, | |
| id = function_id, | |
| v_id = function_version_id, | |
| env = env_suffix, | |
| window = utilization_window_seconds | |
| ), | |
| MetricSource::LlmGateway => { | |
| // Little's Law: average duration * request rate collapses to the | |
| // per-second rate of the duration sum. | |
| format!( | |
| r#"100 * sum by(function_id) (rate(llm_api_gateway_http_request_duration_seconds_sum{{function_id="{id}"}}[2m])) / | |
| clamp_min( | |
| sum by(function_id) (nvcf_function_instances_current{{function_id="{id}"{env}}}) | |
| * on(function_id) group_left() max by(function_id) (nvcf_function_concurrency{{function_id="{id}"{env}}}), | |
| 1 | |
| )"#, | |
| id = function_id, | |
| env = env_suffix, | |
| ) | |
| } | |
| }; |
🤖 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 `@src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs`
around lines 115 - 141, Update the MetricSource::LlmGateway query in the metric
query construction to divide the in-flight request rate by both current
instances and nvcf_function_concurrency, matching the ControlPlane utilization
formula. Preserve the existing function_id grouping and zero-safe denominator
behavior so the result remains a percentage calibrated for decide_scaling.
Comment on lines
+343
to
+351
| fn gateway_target_desired_instances( | ||
| desired_total: usize, | ||
| total_current: usize, | ||
| target_current: usize, | ||
| ) -> usize { | ||
| target_current | ||
| .saturating_add(desired_total) | ||
| .saturating_sub(total_current) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The gateway delta model cannot scale down idle versions, and it can zero the selected version.
gateway_target_desired_instances converts a total desired count into a per-version count with target_current + desired_total - total_current, floored at 0 by saturating_sub. Two failure modes follow, because Line 812 returns early for every version that is not the selected target:
- Non-selected versions never receive a scaling request. Their instances stay allocated indefinitely.
total_currenttherefore stays high, and the delta stays small. - When
total_current - target_current >= desired_total, the target gets 0. Example:total_current = 10split as target 4 and an idle version 6, withdesired_total = 5. The result is 0 for the target, while the idle version keeps 6. The active version drops to zero and the surplus stays.
The test at Line 1147 pins this floor behavior, so the intent is unclear. Either issue scale-down requests for non-selected versions, or clamp the target to at least the count that the shared decision requires. Do you want me to open an issue to track the multi-version scale-down path?
🤖 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 `@src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs`
around lines 343 - 351, The gateway scaling flow must prevent idle versions from
retaining surplus instances and must not reduce the selected version to zero
when shared capacity is redistributed. Update gateway_target_desired_instances
and the surrounding Line 812 per-version handling so non-selected versions
receive scale-down requests, while the selected target retains at least the
count required by the shared desired-total decision; revise the test covering
the current saturating-subtraction floor accordingly.
Comment on lines
+353
to
+417
| async fn get_gateway_target( | ||
| timeseries_db_client: &TimeseriesDbClient, | ||
| function_id: &Uuid, | ||
| routing_cache: &MetricRoutingCache, | ||
| ) -> Result<GatewayTarget> { | ||
| let end_time = Utc::now(); | ||
| let query = format!( | ||
| r#"((max by(function_id, function_version_id) (nvcf_function_instances_current{{function_id="{id}"}})) | ||
| * on(function_id, function_version_id) group_left(nca_id) | ||
| max by(function_id, function_version_id, nca_id) (nvcf_function_info{{function_id="{id}"}})) | ||
| or | ||
| (max by(function_id, function_version_id, nca_id) (nvcf_function_info{{function_id="{id}"}}) * 0)"#, | ||
| id = function_id, | ||
| ); | ||
| let response = timeseries_db_client | ||
| .query_range( | ||
| &query, | ||
| end_time - Duration::minutes(5), | ||
| end_time, | ||
| TIMESERIES_DB_QUERY_STEP, | ||
| ) | ||
| .await?; | ||
|
|
||
| let mut versions = Vec::new(); | ||
| for result in response.data.result { | ||
| let Some(version_id) = result | ||
| .metric | ||
| .function_version_id | ||
| .as_deref() | ||
| .and_then(|value| Uuid::parse_str(value).ok()) | ||
| else { | ||
| continue; | ||
| }; | ||
| let current_instances = result | ||
| .values | ||
| .last() | ||
| .and_then(|(_, value)| value.parse::<f64>().ok()) | ||
| .filter(|value| value.is_finite() && *value >= 0.0) | ||
| .map(|value| value.round() as usize) | ||
| .unwrap_or(0); | ||
| versions.push(GatewayTarget { | ||
| function_version_id: version_id, | ||
| nca_id: result.metric.nca_id.unwrap_or_default(), | ||
| current_instances, | ||
| total_current_instances: 0, | ||
| }); | ||
| } | ||
|
|
||
| let total_current_instances = versions | ||
| .iter() | ||
| .map(|version| version.current_instances) | ||
| .sum(); | ||
| let pinned = routing_cache.gateway_targets.get(function_id); | ||
| let mut target = select_gateway_target(versions, pinned).with_context(|| { | ||
| format!( | ||
| "nvcf_function_info returned no versions for gateway function {}", | ||
| function_id | ||
| ) | ||
| })?; | ||
| target.total_current_instances = total_current_instances; | ||
| routing_cache | ||
| .gateway_targets | ||
| .insert(*function_id, target.function_version_id); | ||
| Ok(target) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The new gateway query path drops environment scoping. Every pre-existing query applies aws_env="{env}" or environment="{env}" unless ignore_env is set. The three new gateway queries do not. If the timeseries backend holds more than one environment, these queries mix environments, and the mixing feeds instance counts, deltas, and discovery.
src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs#L353-L417: add the environment matcher to bothnvcf_function_instances_currentandnvcf_function_infoinget_gateway_target, and passenvandignore_envinto the function. Without it,total_current_instancessums instances from other environments, andgateway_target_desired_instancescomputes the delta from an inflated total.src/control-plane-services/function-autoscaler/crates/server/src/work/discovery.rs#L188-L199: apply the sameaws_envmatcher thatget_timeseries_db_querybuilds, and honorignore_env, so discovery does not insert functions from another environment into this environment's active-function table. Update the assertion at Line 995 to match.src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs#L115-L141: theLlmGatewaynumerator omits{env}while the denominator includes it. Align the two so the ratio uses one scope.
If llm_api_gateway_http_request_duration_seconds_sum and nvcf_function_info genuinely carry no environment label, state that in a comment at each site so the asymmetry is not read as an omission.
📍 Affects 2 files
src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs#L353-L417(this comment)src/control-plane-services/function-autoscaler/crates/server/src/work/discovery.rs#L188-L199
🤖 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 `@src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs`
around lines 353 - 417, Scope all gateway metrics to the current environment:
update get_gateway_target in
src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs
(lines 353-417) to accept env and ignore_env and apply the appropriate matcher
to both nvcf_function_instances_current and nvcf_function_info; align the
LlmGateway numerator and denominator in the same file (lines 115-141), or
document at both sites if either metric lacks an environment label. Update
discovery in
src/control-plane-services/function-autoscaler/crates/server/src/work/discovery.rs
(lines 188-199) to reuse get_timeseries_db_query’s aws_env matcher while
honoring ignore_env, and adjust the assertion at line 995 accordingly.
Comment on lines
+502
to
637
| "No worker series for {}:{} (nca_id={}); caller will try gateway metrics", | ||
| function_id, | ||
| function_version_id, | ||
| nca_id | ||
| ); | ||
| Ok(None) | ||
| } | ||
|
|
||
| /// Resolve the current instance count and which metric family drives scaling. | ||
| /// Worker metrics are the default; when no worker series exists at query time | ||
| /// (BYOC and other CP-only functions) we fall back to control-plane metrics. | ||
| /// Instance-count lookups never abort the cycle — failures degrade to 0. | ||
| async fn resolve_current_instances( | ||
| /// Gather source-specific inputs and retain the selected gateway target for deployment. | ||
| #[allow(clippy::too_many_arguments)] | ||
| async fn gather_scaling_inputs( | ||
| timeseries_db_client: &TimeseriesDbClient, | ||
| function_id: &Uuid, | ||
| function_version_id: &Uuid, | ||
| nca_id: &str, | ||
| env: &str, | ||
| ignore_env: bool, | ||
| ) -> (usize, MetricSource) { | ||
| match get_current_worker_count_from_timeseries_db( | ||
| timeseries_db_client, | ||
| function_id, | ||
| function_version_id, | ||
| nca_id, | ||
| env, | ||
| ignore_env, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(Some(n)) => (n, MetricSource::WorkerThreads), | ||
| Ok(None) => { | ||
| tracing::info!( | ||
| "No worker series for {}:{} (nca_id={}); falling back to control-plane metrics", | ||
| function_id, | ||
| function_version_id, | ||
| nca_id | ||
| ); | ||
| let n = get_byoc_instance_count( | ||
| scaling_settings: &ScalingSettings, | ||
| routing_cache: &MetricRoutingCache, | ||
| ) -> Result<GatheredScalingInputs> { | ||
| let key = (*function_id, *function_version_id); | ||
| let cached_source = routing_cache.sources.get(&key); | ||
| let mut metric_source = cached_source.unwrap_or(MetricSource::WorkerThreads); | ||
| let mut current_instances = 0; | ||
| let mut gateway_target = None; | ||
| let mut cache_source = cached_source.is_none(); | ||
|
|
||
| if metric_source == MetricSource::WorkerThreads { | ||
| match get_current_worker_count_from_timeseries_db( | ||
| timeseries_db_client, | ||
| function_id, | ||
| function_version_id, | ||
| nca_id, | ||
| env, | ||
| ignore_env, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(Some(count)) => current_instances = count, | ||
| Ok(None) if cached_source.is_none() => { | ||
| metric_source = | ||
| if llm_gateway_metrics_present(timeseries_db_client, function_id).await? { | ||
| MetricSource::LlmGateway | ||
| } else { | ||
| MetricSource::ControlPlane | ||
| }; | ||
| } | ||
| Ok(None) => {} | ||
| Err(error) => { | ||
| tracing::warn!( | ||
| "TimeseriesDb worker count failed for {}:{} (nca_id={}), using 0: {}", | ||
| function_id, | ||
| function_version_id, | ||
| nca_id, | ||
| error | ||
| ); | ||
| cache_source = false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| match metric_source { | ||
| MetricSource::WorkerThreads => {} | ||
| MetricSource::LlmGateway => { | ||
| let target = | ||
| get_gateway_target(timeseries_db_client, function_id, routing_cache).await?; | ||
| current_instances = target.total_current_instances; | ||
| gateway_target = Some(target); | ||
| } | ||
| MetricSource::ControlPlane => { | ||
| current_instances = get_byoc_instance_count( | ||
| timeseries_db_client, | ||
| function_id, | ||
| function_version_id, | ||
| env, | ||
| ignore_env, | ||
| ) | ||
| .await | ||
| .unwrap_or_else(|e| { | ||
| .unwrap_or_else(|error| { | ||
| tracing::warn!( | ||
| "CP fallback instance count failed for {}:{} (nca_id={}), using 0: {}", | ||
| "CP instance count failed for {}:{}, using 0: {}", | ||
| function_id, | ||
| function_version_id, | ||
| nca_id, | ||
| e | ||
| error | ||
| ); | ||
| 0 | ||
| }); | ||
| (n, MetricSource::ControlPlane) | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| "TimeseriesDb worker count failed for {}:{} (nca_id={}), using 0: {}", | ||
| function_id, | ||
| function_version_id, | ||
| nca_id, | ||
| e | ||
| ); | ||
| (0, MetricSource::WorkerThreads) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Gather everything the scaling decision needs from the timeseries DB into a | ||
| /// single sanitized struct. After this returns, the decision is identical for | ||
| /// every metric source (see `scaling::decide_scaling`). Utilization is | ||
| /// sanitized here, so downstream logic never sees NaN/Inf or unsorted points. | ||
| async fn gather_scaling_inputs( | ||
| timeseries_db_client: &TimeseriesDbClient, | ||
| function_id: &Uuid, | ||
| function_version_id: &Uuid, | ||
| nca_id: &str, | ||
| env: &str, | ||
| ignore_env: bool, | ||
| scaling_settings: &ScalingSettings, | ||
| ) -> Result<ScalingInputs> { | ||
| let (current_instances, metric_source) = resolve_current_instances( | ||
| timeseries_db_client, | ||
| function_id, | ||
| function_version_id, | ||
| nca_id, | ||
| env, | ||
| ignore_env, | ||
| ) | ||
| .await; | ||
| if cache_source { | ||
| routing_cache.sources.insert(key, metric_source); | ||
| } | ||
|
|
||
| let raw_utilization = get_function_utilization_history( | ||
| timeseries_db_client, | ||
| function_id, | ||
| function_version_id, | ||
| env, | ||
| metric_source.uses_control_plane_metrics(), | ||
| metric_source, | ||
| ignore_env, | ||
| scaling_settings.lookback.as_secs() as i64 / 60, | ||
| scaling_settings.utilization_window_seconds, | ||
| ) | ||
| .await?; | ||
| let utilization_samples = sanitize_utilization(raw_utilization); | ||
|
|
||
| let recent_invocations = get_recently_invoked_functions( | ||
| timeseries_db_client, | ||
| Some(*function_version_id), | ||
| scaling_settings.scale_to_zero_idle_timeout.as_secs() as i64 / 60, | ||
| env, | ||
| ignore_env, | ||
| ) | ||
| .await?; | ||
| let recently_invoked = if metric_source == MetricSource::LlmGateway { | ||
| llm_gateway_recently_invoked( | ||
| timeseries_db_client, | ||
| function_id, | ||
| scaling_settings.scale_to_zero_idle_timeout.as_secs() as i64 / 60, | ||
| ) | ||
| .await? | ||
| } else { | ||
| !get_recently_invoked_functions( | ||
| timeseries_db_client, | ||
| Some(*function_version_id), | ||
| scaling_settings.scale_to_zero_idle_timeout.as_secs() as i64 / 60, | ||
| env, | ||
| ignore_env, | ||
| ) | ||
| .await? | ||
| .is_empty() | ||
| }; | ||
|
|
||
| Ok(ScalingInputs { | ||
| metric_source, | ||
| current_instances, | ||
| utilization_samples, | ||
| recently_invoked: !recent_invocations.is_empty(), | ||
| Ok(GatheredScalingInputs { | ||
| inputs: ScalingInputs { | ||
| metric_source, | ||
| current_instances, | ||
| utilization_samples, | ||
| recently_invoked, | ||
| }, | ||
| gateway_target, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for tracing instrumentation on the timeseries DB client request path.
set -uo pipefail
fd -t f 'timeseries_db_client.rs' --exec rg -n -C4 'instrument|tracing::(span|info_span|debug_span)|fn query_range'
rg -n 'tracing::instrument' --glob 'src/control-plane-services/function-autoscaler/**/*.rs'Repository: NVIDIA/nvcf
Length of output: 1112
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo '--- relevant guidance ---'
find .. -name AGENTS.md -print 2>/dev/null | sort
echo '--- TimeseriesDbClient request path ---'
fd -t f . | rg 'timeseries_db_client\.rs$' | while read -r f; do
sed -n '350,440p' "$f"
done
echo '--- referenced helper definitions ---'
rg -n -C8 'fn (llm_gateway_metrics_present|get_gateway_target|llm_gateway_recently_invoked|get_function_utilization_history|get_current_worker_count_from_timeseries_db|get_byoc_instance_count)' src/control-plane-services/function-autoscaler
echo '--- tracing error attributes in autoscaler ---'
rg -n -C2 'tracing::instrument|span!|error\(' src/control-plane-services/function-autoscalerRepository: NVIDIA/nvcf
Length of output: 34274
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo '--- applicable autoscaler guidance ---'
cat src/control-plane-services/function-autoscaler/AGENTS.md
echo '--- complete request execution path ---'
sed -n '390,520p' src/control-plane-services/function-autoscaler/crates/server/src/timeseries_db/timeseries_db_client.rs
echo '--- helper bodies and query calls ---'
sed -n '90,210p' src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs
sed -n '300,442p' src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs
echo '--- all query_range call sites ---'
rg -n -C4 '\.query_range\(' src/control-plane-services/function-autoscalerRepository: NVIDIA/nvcf
Length of output: 42466
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo '--- tracing dependency version ---'
rg -n -C2 '^tracing(-attributes)?\s*=' src/control-plane-services/function-autoscaler/Cargo.toml src/control-plane-services/function-autoscaler/Cargo.lock Cargo.lock 2>/dev/null || true
echo '--- local tracing instrument documentation ---'
rg -n -C5 'record an error|err.*Result|does not record.*error|return.*Err|To record.*error' \
../.cargo/registry/src --glob '*tracing-attributes*/src/**/*.rs' --glob '*tracing-*/src/**/*.rs' 2>/dev/null | head -200
echo '--- instrument attributes in the client ---'
rg -n -C3 '#\[tracing::instrument' src/control-plane-services/function-autoscaler/crates/server/src/timeseries_db/timeseries_db_client.rsRepository: NVIDIA/nvcf
Length of output: 559
🌐 Web query:
tracing-attributes #[instrument] err attribute returned Err default error recording
💡 Result:
When using the #[instrument] attribute macro from the tracing crate with the err argument, the macro automatically emits a tracing event if the instrumented function returns a Result::Err [1][2]. The default behavior and options for error recording are as follows: Default Formatting: By default, the error value is recorded using its std::fmt::Display implementation [1][3]. This is equivalent to invoking tracing::error!(error = %e) [4][5]. Alternative Formatting: You can change how the error is formatted by specifying a mode within the err attribute [1][3]: - err(Display): Uses the Display implementation (the same as the default) [1]. - err(Debug): Uses the Debug implementation [1][3]. This is equivalent to invoking tracing::error!(error =?e) [4][5]. - err(StdError): Records the error as a &dyn std::error::Error [5]. This is useful for integration with systems like OpenTelemetry that can extract error chains, though it requires the error type to implement std::error::Error [5]. Default Level: By default, the emitted error event is logged at the ERROR level [1][2]. Customizing Level: While the primary err argument defaults to ERROR, you can override the level by providing it as a parameter, such as #[instrument(err(level = Level::INFO))] [1][2]. Note that err and err(Debug) are mutually exclusive; attempting to use both will result in a compilation error [4]. Additionally, if you are using the ret argument alongside err, the macro will record the return value if it is Result::Ok and the error value if it is Result::Err [1].
Citations:
- 1: https://docs.rs/tracing/latest/tracing/attr.instrument.html
- 2: https://docs.rs/tracing-attributes/latest/src/tracing_attributes/lib.rs.html
- 3: https://docs.rs/tracing/0.1.37/tracing/attr.instrument.html
- 4: tokio-rs/tracing@a1868ea
- 5: attributes: support recording error as
std::error::Errortokio-rs/tracing#3057
Record TimeseriesDb errors in the existing request span. All three helpers reach execute_request through query_range, so the request span covers them. Add err to #[tracing::instrument] because returned Err values are not recorded without it.
🤖 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 `@src/control-plane-services/function-autoscaler/crates/server/src/work/mod.rs`
around lines 502 - 637, The TimeseriesDb helper errors are not currently
recorded in the request span. Update the existing tracing instrumentation on the
shared request execution function reached via query_range to include the
returned error field using err, preserving the existing helper behavior and span
coverage.
Source: Coding guidelines
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.
TL;DR
Add LLM API gateway metrics as an autoscaling source between worker metrics and the existing control-plane fallback.
The autoscaler now prefers worker metrics, then LLM gateway metrics, and finally control-plane metrics. The selected source is cached for one hour.
Additional Details (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
nvcf_function_info.For the Reviewer
Please focus on:
work/mod.rs;work/discovery.rs;For QA (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
Validated with:
cargo fmt -p rs-autoscaler -- --checkcargo check -p rs-autoscaler --all-targetscargo clippy -p rs-autoscaler --all-targets -- -D warningscargo test -p rs-autoscalerIssues
NO-REF
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests