Skip to content

feat(eap): collect more cluster load signals for storage routing - #8466

Draft
pbhandari wants to merge 1 commit into
masterfrom
feat/get_more_clusterinfo
Draft

pbhandari wants to merge 1 commit into
masterfrom
feat/get_more_clusterinfo

Conversation

@pbhandari

@pbhandari pbhandari commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

get_cluster_loadinfo now returns a wider LoadInfo bag so we can graph hottest-replica load and pick thresholds for a later routing / dynamic-sampling strategy. This PR is probe + gauges only; nothing branches on the new fields yet.

Signals (all float, default -1.0):

  • cluster_load — max 1m load / CPU, as before
  • concurrent_queries — max system.metrics.Query (was count() on system.processes)
  • cgroup_user_time_normalized
  • block_in_flight_ops
  • memory_tracking
  • part_mutation

Replica aggregations are max (the hot node). Missing CH metrics and old Redis blobs become -1.0; treat that as not-loaded. Probe failure still increments get_cluster_loadinfo_failure and does not gauge.

One INTERNAL query per cache miss. Single-node vs clusterAllReplicas SQL is unchanged.

Follow-up: look at the gauges, set thresholds, then a strategy that actually reads these.

@pbhandari
pbhandari requested review from a team as code owners September 11, 2026 21:09
return load_info

except Exception as e:
metrics.increment("get_cluster_loadinfo_failure", tags={"cluster_name": cluster_name})

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.

Bug: If get_cluster() fails, the except block references cluster_name before it's assigned, causing an UnboundLocalError and masking the original exception.
Severity: MEDIUM

Suggested Fix

Initialize cluster_name to a default value like None before the try block. This ensures the variable is always defined when the except block is executed, preventing the UnboundLocalError.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: snuba/web/rpc/storage_routing/load_retriever.py#L169

Potential issue: In the `get_cluster_loadinfo` function, if
`get_cluster(storage_set_key)` raises an exception, such as `UndefinedClickhouseCluster`
due to a misconfiguration, the `cluster_name` variable is never assigned. The `except`
block then attempts to reference `cluster_name` when creating a metric tag. This will
raise an `UnboundLocalError`, which masks the original, more informative exception about
the cluster configuration problem, making debugging more difficult.

Did we get this right? 👍 / 👎 to inform future reviews.

@pbhandari
pbhandari marked this pull request as draft September 11, 2026 21:13

This branch has not been deployed

No deployments
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.

1 participant