graphite: expand globs and braces in render targets for health and flag metrics

[sergiubz]

Grafana sends {a,b,c} for a multi-value variable and * for All, but /render only matched exact service names (plus a trailing * on flag metrics), so every such panel and the health.<env>.* alert rules on dashboard.tsi-dev came back empty; the health and flag targets are now expanded against the configured services with the same glob syntax Graphite uses, exact names behave as before, and the find endpoint is untouched.

[ecosquad-autoreview]

Error

An error occurred while processing your request:

Model response did not contain a tool call even though tool choice was required.

[LukasCuperDT]

Nice change and good tests. A few things before merge:

1. Unbounded expansion (DoS). expand_braces produces the cartesian product of every {…} group in a user-supplied target. health.prod.{a,b}{a,b}… with 25 groups yields ~33M strings from one unauthenticated request. Please cap the number of groups and/or total expansions (and reject or return [] beyond it).
2. Sequential backend calls. health.<env>.* awaits get_service_health once per service in sequence, so N services = N round-trips in a row. Something like futures::stream::iter(..).buffer_unordered(k) (or join_all with a bound) would keep "All" panels responsive.
3. Semantics vs Graphite — worth a test or a doc comment:
   • glob::Pattern lets * match across .; Graphite stops at ..
   • Nested braces ({a,{b,c}}) aren't handled.
   • The target is split on . before expansion, so a brace member containing a dot breaks.

[LukasCuperDT]

Requesting changes per my earlier review: cap brace expansion (currently unbounded cartesian product from user input) and avoid sequential per-service backend calls for globbed health targets.

[sergiubz]

Addressed in df81808:

1. Expansion cap – a target may carry at most 8 brace groups and expand to at most 256 alternatives; anything beyond matches nothing ([]), checked before the cartesian product is built. Tests cover 9 groups, 8 groups at the limit and 5×4 members.
2. Concurrency – health.<env>.<glob> now runs the per-service fetches through futures::stream::iter(..).buffered(8), so "All" is bounded at 8 in flight and the series keep the order of the known services.
3. Semantics – matching is now node by node: the pattern and the candidate are split on . and each node is matched with its own glob::Pattern, so */? stop at a dot exactly like Graphite (test test_expand_pattern_follows_graphite_node_semantics). Nested groups ({a,{b,c}}) are expanded by tracking brace depth and splitting on top-level commas. A brace member containing a dot stays unsupported because the handler splits the target first; that is documented on expand_pattern.

cargo fmt --check, cargo clippy -- -D warnings and cargo test --all-targets (80 passed) are green locally.

[ecosquad-autoreview]

Summary

Adds Graphite-style glob/brace expansion to the /render handler for flag.* and health.* targets (src/graphite.rs), replacing the old exact-match + trailing-* logic, and adds a futures dependency for stream-based concurrent health fetches. The intent is sound: health.<env>.* and {a,b,c} multi-value targets now expand against the configured services. However, the reviewed revision has a few concerns that should be verified before merging (CI build/test are still queued, so none of this is compile- or runtime-verified).

Findings

Warning

1. Health branch appears to have lost its early return — no-match / error behavior is unclear (src/graphite.rs, ~line 347–375, the target_parts.len() == 3 health block).
   The old code returned a 200 immediately when get_service_health succeeded, and fell through to the generic Graphite path on error. The new code builds series via stream::iter(expand_pattern(...)).map(...). I can only see it collect into series, with no visible early return. If the code now always returns series (even when empty) or falls through to the Graphite branch, the behavior changes:

   • A target that matches no configured service would previously fall through to Graphite (which would 404), but now may return an empty 200 response (or hit the Graphite path with an unexpanded brace string).
   • The previous "error → fall through to Graphite" path should be intentional; confirm it is preserved or explicitly 404/500.
     Fix: make the fallthrough behavior explicit — if expand_pattern returns empty, return 404 (matching Graphite semantics) rather than 200 [], and preserve/replace the error-fallthrough with a deliberate response.

2. expect on required query fields panics the handler (src/graphite.rs, handler_render, ~lines 235–250).
   max_data_points.expect(" is required"), query.target.as_ref().expect("Target is required"), etc. will panic (500) on a malformed request instead of returning a clean 400. Pre-existing, but this PR restructures the handler, so it's a good moment to replace these with Option propagation and StatusCode::BAD_REQUEST.

Suggestion

3. Unbounded concurrency in health fan-out. The stream::iter(...).map(...) that fetches get_service_health per expanded service will issue one request per matched service. For a large multi-value variable or a broad glob, this is unbounded. Consider buffer_unordered(N) with a modest N to bound in-flight requests.

4. New expand_pattern helper (src/graphite.rs:409) is not fully visible in this review — I could not inspect its glob/brace semantics (e.g., whether {a,b,c} at a non-last position is handled, escaping, or empty-expansion cases). The flag branch (flag.<env>.{a,b,c}) relies on it producing exact service.metric keys that then match state.flag_metrics; verify it produces exactly the key format the maps use (e.g., svc.flag vs flag.svc). Worth a unit test for expand_pattern covering *, {a,b,c}, and literal names.

Verdict

The core change is reasonable, but the health-branch control flow (finding #1) needs to be confirmed before this merges. I'd like to see the fallthrough/empty-match behavior resolved.

I could not run the build or tests (CI is queued), so the above is based on the diff and surrounding code only.

[sergiubz]

Checked against df81808, the reviewed revision:

1. The health branch still ends with an explicit return (StatusCode::OK, Json(json!(series))). There is no Graphite fallthrough in this handler: before the PR an error in get_service_health fell through to the end of handler_render, which returns 200 [], and a target matching nothing returns the same 200 [] now. Behaviour unchanged; a failing service is now omitted instead of blanking the whole response.
2. Valid but pre-existing (expect on target, maxDataPoints, from, until); Grafana always sends them. Will be handled in a separate PR to keep this one focused.
3. Already bounded: the fan-out runs through .buffered(HEALTH_FETCH_CONCURRENCY) with a limit of 8, in the same block.
4. expand_pattern is covered by five unit tests (test_expand_pattern_*, test_expand_braces) plus two handler tests; the flag keys are service.metric, built the same way as in handler_metrics_find and verified by the existing test_render_flag_target.

CI on this revision: fmt, clippy -D warnings and 80 tests green.

[LukasCuperDT]

Thanks for df81808 — bounded concurrency, per-node glob matching and nested braces look good, and I agree with your answers to the autoreview (200 [] unchanged; the expect()s are pre-existing).

One blocker left: the 256 cap is checked after the product is built. expand_pattern calls expand_braces(pattern), which materialises every combination, and only then compares alternatives.len() with MAX_PATTERN_EXPANSIONS. MAX_BRACE_GROUPS limits the number of groups, not members per group: 8 groups × 30 members = 30⁸ ≈ 6.5×10¹¹ strings from a ~500-character query.

Please bound it before expanding, e.g. multiply the top-level member counts of each group (recursively for nested ones) and return [] once the product exceeds 256, or thread a remaining budget through expand_braces and stop as soon as it is exhausted. A test with something like 8 groups × 30 members that returns [] quickly would lock it in.

[ecosquad-autoreview]

Summary

This PR adds Graphite-style glob + brace expansion to the /render endpoint for flag.<env>.<service>.<metric> and health.<env>.<service> targets, so that Grafana multi-value variables ({a,b,c}) and * (All) resolve against configured services instead of returning empty. New helpers expand_pattern / expand_braces / expand_braces_within are added with explicit expansion budgets (MAX_BRACE_GROUPS = 8, MAX_PATTERN_EXPANSIONS), plus a set of unit and integration tests. Cargo.toml adds a futures dependency for the async fan-out on the health path.

I could only see a truncated portion of src/graphite.rs (the full bodies of the health fan-out loop and the expand_* helpers were cut off), and CI build/test/Analyze are still in progress, so I could not confirm compilation or run the tests. The items below are scoped to what I was able to verify; please re-check the ones I marked as "verify" once you have the full file in front of you.

Findings

Warning — potential unbounded concurrency on the health.<env>.* path (verify)

src/graphite.rs ~line 352: the health target now does

let series: Vec<serde_json::Value> = stream::iter(expand_pattern(target_parts[2], &known))
    .map(|s| ... get_service_health(...)) ...

When a panel issues health.<env>.*, expand_pattern can match every configured service, and if the stream is collected with join_all / buffer_unordered without a cap, all backend get_service_health calls are issued simultaneously. With a large service set this can saturate the TSDB connection pool / cause timeouts under load.
Fix: cap the fan-out, e.g. .buffer_unordered(N) or .buffered(N) with a small constant (e.g. 8–16), and confirm the response still includes all services (only ordering changes).

Warning — .expect() on required render fields panics the request (pre-existing, now more reachable)

src/graphite.rs handler_render:

x.max_data_points.expect(" is required")      // and
query.max_data_points.expect("Query is required")

A /render request without maxDataPoints panics (500) rather than returning a clean 400. This predates the PR, but the glob/brace expansion makes /render a much hotter path, so it's worth converting to a proper Err(StatusCode::BAD_REQUEST) rejection while you're in the file.

Suggestion — glob semantics on the flag path changed from starts_with to glob match

src/graphite.rs ~line 280: the previous code did a plain metric.starts_with(target) when the metric name ended in *. It is now expand_pattern(&metric_name, &known) using glob matching. The tests (test_expand_pattern_exact_name_is_returned_unchanged) confirm exact names are unchanged and */?/[...] behave as Graphite expects, which is the intent. Just double-check that no existing dashboard relied on the old starts_with-on-trailing-* behavior in a way glob doesn't cover (e.g. a trailing * was previously a pure prefix match; glob * matches any suffix within a segment). The added tests look thorough, so this is likely fine — flagging for confirmation.

Suggestion — good: expansion budget guards against DoS

MAX_BRACE_GROUPS = 8 and the expand_braces_within limit (returning None/empty once exceeded) are a sensible defense against pathological brace expressions (e.g. a large {a,b,c,...} in a target). The tests test_expand_pattern_caps_brace_expansion cover this. No change needed.

Notes / open items

• CI is pending: build, test, and Analyze (rust) were in_progress at review time, so I could not confirm the code compiles (new futures dep + stream::iter/StreamExt usage) or that the new tests pass. Please confirm they go green before merging.
• I could not read the full bodies of expand_pattern / expand_braces_within due to output truncation; the brace-nesting and stray-brace handling (test_expand_braces) appears to be well tested, but a final look at the helper implementations is warranted for edge cases like an unclosed bracket within a brace member.

No correctness blockers were identified in what I could verify; the design (glob + brace expansion with explicit budgets, exact names preserved, find endpoint untouched) matches the stated goal.

[sergiubz]

Agreed, done in 2ebd2f8: the budget is threaded through the recursion so expansion stops as soon as it would exceed 256 alternatives, and the cap test now covers 8 groups x 30 members returning [] quickly.