[APM] Harden Services, Overview, Operations, Dependencies, and Topology pages for scale

[ps48]

Summary

This PR hardens all five APM pages (Services home, Service overview, Service operations, Service dependencies, Topology map) at high service and environment cardinality (hundreds of services across many environments), where query construction, request concurrency, and rendering issues surface. The changes came from an audit of every PromQL and PPL query builder plus the data hooks and page components.

Problems addressed

1. Oversized PromQL queries

The Services home and Topology pages built a service=~"a|b|c|..." filter from the entire service list and embedded it into each metric query. The failure rate query embedded it three times, so at a few hundred services it crossed the datasource query-length limit and returned HTTP 400. That blanked the failure rate column, and because throughput shared an all-or-nothing Promise.all with it, throughput went blank too.

Fix: rely on the existing sum by (service) grouping instead of a service list filter, so query length is constant regardless of fleet size. The service filter is now optional and omitted when empty.

2. All-or-nothing metric fetching

Services home, Operations, Dependencies, and Topology fetched their metric queries with Promise.all, so one failing query blanked every column, not just its own.

Fix: Promise.allSettled with per-metric handling, so a single failure only affects its own column.

3. Racy renders on refresh

No APM data hook cancelled or guarded in-flight requests, so a response from a superseded refresh or time range change could overwrite fresh state. On Services home this manifested as the catalog collapsing to a single service on some refreshes, because an auto-derived filter range computed from partial data was copied into the active filter.

Fix: AbortController based stale-response guards in every data hook (matching the existing pattern in use_apm_config.ts), an optional AbortSignal threaded through the PromQL and PPL search services, gating the Services home / Operations / Dependencies range filters on whether the user actually moved the slider, and waiting for metrics to settle before syncing bounds. Also fixed a Topology refresh that fetched the map twice.

4. Query correctness and safety

• Application and group builders now filter remoteService="" so server span metrics are not double counted with client spans, and latency histograms are not mixed across span kinds.
• Added PromQL label, PromQL regex, and PPL string escape helpers and applied them to interpolated service, environment, operation, and remote values.
• Added a row cap to the unbounded PPL list and service map queries, and removed two dead PPL builders.
• Reconciled the environment value passed to the Operations list and metrics hooks.

5. Multi-environment metrics and failure-rate accuracy

• Services home metric queries grouped by service only, so a service running in multiple environments had its throughput, latency, and failure rate summed into one value shown on every environment row. The catalog builders now group by (environment, service) and the metrics map keys on both.
• The catalog failure rate averaged the per-step ratio (over-weighting low-traffic steps); it now uses a ratio of summed totals over the range.

6. Service overview P99 latency card

The P99 card averaged per-step P99 values (labelled "Avg"), which is not meaningful for a percentile. It now shows the P99 over the selected range and drops the misleading subtitle.

7. Topology rendering

The graph rebuilt all nodes and ran a full dagre relayout on every metrics tick and every edge selection.

Fix: memoize the structural layout separately from the metric and selection overlays so a metrics update or edge click no longer triggers relayout, plus a node cap with a clear notice above it.

8. Catalog sparklines fetched per visible page

The per-step range queries that back the row sparklines were fetched for every service, even though sparklines are only shown for the rows on the current page. Instant metrics (numbers, sort, filter) still load for all services; the range/sparkline queries now run only for the visible page via a bounded, escaped service=~ filter.

The visible-page fetch is cached and incremental: revisiting a page whose series are already in memory issues no new request, only the not-yet-fetched services on a page are queried, page changes are debounced so skimming past pages does not fire a request per intermediate page, and superseded fetches are aborted. A time range, percentile, or refresh change clears the cache so no stale series remain.

Follow ups (intentionally not in this PR)

These were found in the audit but need product decisions or live data validation and are better as separate changes: aligning the throughput unit across all surfaces, windowing the remaining Service overview RED cards over the selected range, and reconciling the availability (5xx only) vs failure ratio (4xx and 5xx) definitions.

Testing

• Built and ran the plugin against a local OpenSearch stack fed with synthetic services (hundreds of services across several environments).
• Verified with Chrome DevTools network capture that the Services page loads at that scale, the previously failing queries are now small and correctly formed and stay under the datasource query-length limit, and per-environment rows show distinct values.
• Verified the sparkline fetch behaviour on the network panel: page load fetches instant metrics for all services plus range queries scoped to the visible page; paging fetches only the new page's range queries; revisiting a cached page fetches nothing; skimming across several pages fetches only the page that is landed on; and a refresh clears the cache and refetches.
• eslint clean on all changed files.

Notes

No user facing strings changed beyond the Topology "too many services" notice and the Service overview P99 card subtitle.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 9a71e59)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Query escaping + PromQL/PPL query builder changes Relevant files: public/components/apm/query_services/query_requests/escape_utils.ts public/components/apm/query_services/query_requests/ppl_queries.ts public/components/apm/query_services/query_requests/promql_queries.ts public/components/apm/shared/hooks/use_group_metrics.ts Sub-PR theme: AbortController + Promise.allSettled hardening for data hooks Relevant files: public/components/apm/common/types/prometheus_types.ts public/components/apm/query_services/ppl_search_service.ts public/components/apm/query_services/promql_search_service.ts public/components/apm/shared/hooks/use_dependencies.ts public/components/apm/shared/hooks/use_dependency_metrics.ts public/components/apm/shared/hooks/use_operations.ts public/components/apm/shared/hooks/use_operation_metrics.ts public/components/apm/shared/hooks/use_promql_chart_data.ts public/components/apm/shared/hooks/use_selected_edge_metrics.ts public/components/apm/shared/hooks/use_service_dependencies_by_fault_rate.ts public/components/apm/shared/hooks/use_service_map.ts public/components/apm/shared/hooks/use_service_map_metrics.ts Sub-PR theme: Topology map render-cap and layout-recompute fixes Relevant files: public/components/apm/common/constants.ts public/components/apm/pages/application_map/application_map_page.tsx public/components/apm/shared/components/service_map/service_map_graph.tsx

⚡ Recommended focus areas for review Stale cache guard skips fetch Effect 3 depends on throughputFailureMap, and Effect that clears the cache on time/percentile/refresh change runs asynchronously. If Effect 3 runs before the clearing effect completes, the "missing" check sees the old cache and returns early with missing.length === 0, leaving sparklines stale after a refresh or time-range change. Consider deriving cache-invalidation via a version key (e.g., include startTimeSec/endTimeSec/percentile/refetchAllTrigger in the map key or a generation counter) instead of clearing via a separate effect. useEffect(() => { setThroughputFailureMap(new Map()); setLatencyMap(new Map()); }, [startTimeSec, endTimeSec, params.latencyPercentile, refetchAllTrigger]); // Effect 3: sparkline (per-step range) data for the visible page's services, // via a bounded service=~ filter. Results ACCUMULATE across pages, so // revisiting a page is served from memory with no request. Only services not // already cached are fetched. Debounced so skimming pages fires one request // for the page landed on; superseded in-flight fetches are aborted. useEffect(() => { if (!promqlService || sparklineNames.length === 0) return; const visible = params.sparklineServices ?? []; const missing = visible.filter( (s) => !throughputFailureMap.has(serviceNodeKey(s.serviceName, s.environment)) ); if (missing.length === 0) return; // whole page already cached -> no request Possible PromQL length regression The sparkline fetch (Effect 3) still builds service=~"a|b|..." from the visible page. This is bounded by page size, but on very large page sizes combined with long service names it can approach limits again. Consider capping the number of names embedded per request, or chunking, to keep length predictable regardless of page-size configuration. const missingNames = Array.from(new Set(missing.map((s) => s.serviceName))); const filter = `service=~"${missingNames.map(escapePromQLRegex).join('|')}"`; const throughputQuery = getQueryServicesThroughput(filter); const failureRatioQuery = getQueryServicesFailureRatio(filter); const latencyQuery = getQueryServicesLatency(filter, percentileValue); const step = calculateStep(startTimeSec, endTimeSec, RESOLUTION_LOW); Sort mirror uses displayedServices The visible-services effect sorts displayedServices and slices by table page, but the EuiBasicTable is uncontrolled and applies its own default serviceName asc sort. If EUI's internal sort ordering differs from the manual comparator here (e.g., locale-aware collation, null handling, or when metric values are equal), visibleServices will not match the actual visible rows and some rows will show empty sparklines while off-screen ones are fetched. useEffect(() => { const getSortVal = (item: ServiceTableItem): string | number => { const m = metricsMap.get(serviceNodeKey(item.serviceName, item.environment)); switch (tableSortField) { case 'latency': return m?.avgLatency ?? -1; case 'throughput': return m?.avgThroughput ?? -1; case 'failureRatio': return m?.avgFailureRatio ?? -1; case 'environment': return item.environment ?? ''; default: return item.serviceName ?? ''; } }; const sorted = [...displayedServices].sort((a, b) => { const va = getSortVal(a); const vb = getSortVal(b); const cmp = va < vb ? -1 : va > vb ? 1 : 0; return tableSortDirection === 'desc' ? -cmp : cmp; }); const start = tablePageIndex * tablePageSize; const slice = sorted.slice(start, start + tablePageSize); setVisibleServices((prev) => { const sameKeys = prev.length === slice.length && prev.every( (p, i) => p.serviceName === slice[i].serviceName && p.environment === slice[i].environment ); return sameKeys ? prev : slice.map((s) => ({ serviceName: s.serviceName, environment: s.environment })); }); }, [ displayedServices, metricsMap, tableSortField, tableSortDirection, tablePageIndex, tablePageSize, ]); Missing dep in sparkline effect params.sparklineServices is read inside Effect 3 but not listed as a dependency (only sparklineKey derived from names is). If two different sparklineServices arrays share the same set of serviceNames but differ in environment, cache lookups via serviceNodeKey(name, env) won't match and the effect will refetch or miss cache entries. Include environment in sparklineKey for consistency with the composite key used elsewhere. const sparklineNames = Array.from( new Set((params.sparklineServices ?? []).map((s) => s.serviceName)) ); const sparklineKey = sparklineNames.join('|');

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 9a71e59

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Decouple sparkline fetch from cache map This effect lists throughputFailureMap in its deps and calls setThroughputFailureMap inside; after the merge, the map updates cause the effect to re-run, the cache check now passes (missing.length === 0) and it returns — but this still schedules and immediately aborts a timer/AbortController on every cache update, and any future prop change while a fetch is in flight will abort it via the cleanup even if params didn't change. Consider tracking pending fetches by key in a ref instead of depending on the whole throughputFailureMap. public/components/apm/shared/hooks/use_services_red_metrics.ts [325-328] +// Track already-fetched keys in a ref to decouple the fetch effect from cache map updates. +const fetchedSparklineKeysRef = useRef<Set<string>>(new Set()); +// ... in effect: const missing = visible.filter( - (s) => !throughputFailureMap.has(serviceNodeKey(s.serviceName, s.environment)) + (s) => !fetchedSparklineKeysRef.current.has(serviceNodeKey(s.serviceName, s.environment)) ); -if (missing.length === 0) return; // whole page already cached -> no request +if (missing.length === 0) return; +// after successful merge: +missing.forEach(({ serviceName, environment }) => + fetchedSparklineKeysRef.current.add(serviceNodeKey(serviceName, environment)) +); Suggestion importance[1-10]: 6 __ Why: Valid concern: including throughputFailureMap in the deps causes the effect to re-run on every cache update, scheduling and aborting timers unnecessarily. Using a ref-based tracking approach would be cleaner and more efficient.	Low
	Memoize derived sparkline identifiers sparklineNames and sparklineKey are recomputed on every render as new array/string references, which can cause the effect that depends on sparklineKey to re-evaluate unnecessarily and any downstream memo/effect dependencies to churn. Wrap them in useMemo keyed on params.sparklineServices for stability. public/components/apm/shared/hooks/use_services_red_metrics.ts [146-149] -const sparklineNames = Array.from( - new Set((params.sparklineServices ?? []).map((s) => s.serviceName)) +const sparklineNames = useMemo( + () => Array.from(new Set((params.sparklineServices ?? []).map((s) => s.serviceName))), + [params.sparklineServices] ); -const sparklineKey = sparklineNames.join('|'); +const sparklineKey = useMemo(() => sparklineNames.join('|'), [sparklineNames]); Suggestion importance[1-10]: 4 __ Why: Memoizing sparklineNames and sparklineKey is a minor optimization; since sparklineKey is a string used as an effect dep, string equality prevents unnecessary re-runs regardless, but memoizing prevents recomputation each render.	Low
	Clarify composite-key variable naming The variable is named serviceName but it is now a composite key (service::environment) from serviceNodeKey. Passing this composite string to consumers that treat it as a plain service name (e.g., anywhere metricsMap.get(item.serviceName) still exists) will silently miss. Rename the local variable to key for clarity and audit any remaining call sites that used the previous plain-name lookup. public/components/apm/shared/hooks/use_services_red_metrics.ts [430-438] -const serviceNames = new Set([ +const keys = new Set([ ...totalCountMap.keys(), ...latencyInstantMap.keys(), ...failureRatioInstantMap.keys(), ]); -serviceNames.forEach((serviceName) => { - const latencyData = latencyMap.get(serviceName) || []; - const throughputData = throughputFailureMap.get(serviceName)?.throughput || []; +keys.forEach((key) => { + const latencyData = latencyMap.get(key) || []; + const throughputData = throughputFailureMap.get(key)?.throughput || []; Suggestion importance[1-10]: 3 __ Why: The naming issue is real (variable named serviceName now holds a composite key), but this is a readability/maintainability concern rather than a functional bug. The audit request is speculative.	Low
Possible issue	Guard against non-finite slider bounds metricRanges can contain Infinity/-Infinity (initial values before any service has metrics) which will be pushed into the range sliders and any subsequent filter comparisons. Guard against non-finite bounds before syncing so sliders don't receive Infinity. public/components/apm/pages/services_home/services_home.tsx [693-701] if (metricsLoading || metricsMap.size === 0) return; + const { latencyMin, latencyMax, throughputMin, throughputMax } = metricRanges; + if (!Number.isFinite(latencyMin) || !Number.isFinite(latencyMax)) return; + if (!Number.isFinite(throughputMin) || !Number.isFinite(throughputMax)) return; if (!latencyUserModified.current) { - setLatencyRange([metricRanges.latencyMin, metricRanges.latencyMax]); + setLatencyRange([latencyMin, latencyMax]); } if (!throughputUserModified.current) { - setThroughputRange([metricRanges.throughputMin, metricRanges.throughputMax]); + setThroughputRange([throughputMin, throughputMax]); } }, [metricRanges, metricsLoading, metricsMap]); Suggestion importance[1-10]: 6 __ Why: Valid defensive check: metricRanges initialized with Infinity/-Infinity could propagate to sliders if metricsMap.size > 0 but no services have metrics. Reasonable guard to add.	Low

---

Previous suggestions

Suggestions up to commit 49034aa

Category	Suggestion	Impact
Possible issue	Guarantee loading state clears on error Promise.allSettled was substituted for Promise.all, but the surrounding try/catch was removed entirely. If params.dependencies.forEach or the extract helpers throw synchronously (e.g. malformed response), the error will now propagate as an unhandled promise rejection and setIsLoading(false) will never be called, leaving the UI stuck in a loading state. Wrap the body in a try/finally to guarantee setIsLoading(false). public/components/apm/shared/hooks/use_dependency_metrics.ts [161-166] - fetchMetrics(); + const fetchMetrics = async () => { + setIsLoading(true); + setError(null); + try { + // ... existing logic ... + } catch (err) { + if (abortController.signal.aborted) return; + console.error('[useDependencyMetrics] Unexpected error:', err); + setError(err instanceof Error ? err : new Error('Unknown error')); + } finally { + if (!abortController.signal.aborted) setIsLoading(false); + } + }; - return () => abortController.abort(); -}, [ - params.dependencies, - params.serviceName, - params.environment, - Suggestion importance[1-10]: 7 __ Why: Valid concern: the try/catch was removed and any synchronous throw during extraction or map population would result in unhandled rejection and stuck loading state. Wrapping in try/finally improves reliability.	Medium
	Guard against non-finite metric bounds When fullyFilteredItems is empty, metricRanges falls back to Infinity/-Infinity bounds, but the guard here only skips when metricsMap.size === 0. If metrics have loaded but the current filter yields no items, this will set slider ranges to [Infinity, -Infinity]. Add a bound-sanity check to prevent slider corruption. public/components/apm/pages/services_home/services_home.tsx [693-701] useEffect(() => { if (metricsLoading || metricsMap.size === 0) return; + if (!Number.isFinite(metricRanges.latencyMin) || !Number.isFinite(metricRanges.latencyMax)) { + return; + } if (!latencyUserModified.current) { setLatencyRange([metricRanges.latencyMin, metricRanges.latencyMax]); } if (!throughputUserModified.current) { setThroughputRange([metricRanges.throughputMin, metricRanges.throughputMax]); } }, [metricRanges, metricsLoading, metricsMap]); Suggestion importance[1-10]: 6 __ Why: A legitimate edge case: if fullyFilteredItems is empty while metricsMap has entries, the reduce fallback would yield Infinity/-Infinity, potentially corrupting slider state. The guard adds robustness.	Low
General	Memoize derived sparkline values to avoid re-renders sparklineNames, sparklineKey, and sparklineFilter are recomputed on every render (not memoized), producing new array/string references each render. Since sparklineKey is a dependency of the sparkline effect but params.sparklineServices produces a fresh array reference on every parent render, wrap these in useMemo to avoid unnecessary re-renders and potential redundant fetches. public/components/apm/shared/hooks/use_services_red_metrics.ts [142-148] -// Unique service names on the visible page -> a bounded `service=~"..."` -// filter for the sparkline range queries. Small (<=page size), so it stays -// well under the 10,000-char PromQL limit. sparklineKey drives refetch. -const sparklineNames = Array.from( - new Set((params.sparklineServices ?? []).map((s) => s.serviceName)) +const sparklineNames = useMemo( + () => Array.from(new Set((params.sparklineServices ?? []).map((s) => s.serviceName))), + [params.sparklineServices] ); -const sparklineKey = sparklineNames.join('|'); -const sparklineFilter = sparklineNames.length - ? `service=~"${sparklineNames.map(escapePromQLRegex).join('|')}"` - : ''; +const sparklineKey = useMemo(() => sparklineNames.join('|'), [sparklineNames]); +const sparklineFilter = useMemo( + () => + sparklineNames.length + ? `service=~"${sparklineNames.map(escapePromQLRegex).join('|')}"` + : '', + [sparklineNames] +); Suggestion importance[1-10]: 5 __ Why: Memoizing sparklineNames, sparklineKey, and sparklineFilter is a reasonable optimization since params.sparklineServices will produce a new array reference each render, but the effect uses sparklineKey (a string) as its dependency, so re-renders wouldn't actually cause redundant fetches. Minor code cleanliness improvement.	Low
	Verify consistent composite key usage across maps Keys are now composite ${serviceName}::${environment} (via serviceNodeKey), but extractServiceData is still called with a bare serviceName when populating latencyMap/throughputFailureMap, and the combining loop uses composite keys everywhere. Verify that the sparkline maps and instant maps use identical composite keys; the latencyMap.get(serviceName) / throughputFailureMap.get(serviceName) lookups below iterate on serviceNames (composite keys) so they will correctly resolve, but ensure no callsite still expects bare-name keys. public/components/apm/shared/hooks/use_services_red_metrics.ts [397-401] -// Iterate the full service set from the instant maps (all services); the -// sparkline maps (latencyMap / throughputFailureMap) only cover the visible -// page, so off-page rows keep their numbers with empty sparklines. const serviceNames = new Set([ ...totalCountMap.keys(), ...latencyInstantMap.keys(), ...failureRatioInstantMap.keys(), ]); Suggestion importance[1-10]: 2 __ Why: The suggestion is essentially asking the author to verify a change and does not propose a concrete code modification (existing_code equals improved_code). Low impact.	Low

Suggestions up to commit a7b10e5

Category	Suggestion	Impact
Possible issue	Guard slider ranges against Infinity metricRanges can contain Infinity/-Infinity when fullyFilteredItems is empty or no matching metrics exist (the inner reduce leaves the initial Infinity unchanged, and the subsequent Math.floor(Infinity) yields Infinity). Setting slider ranges to Infinity will break the EuiDualRange component. Guard against non-finite values before applying. public/components/apm/pages/services_home/services_home.tsx [693-701] useEffect(() => { if (metricsLoading || metricsMap.size === 0) return; - if (!latencyUserModified.current) { - setLatencyRange([metricRanges.latencyMin, metricRanges.latencyMax]); + const { latencyMin, latencyMax, throughputMin, throughputMax } = metricRanges; + if ( + !latencyUserModified.current && + Number.isFinite(latencyMin) && + Number.isFinite(latencyMax) + ) { + setLatencyRange([latencyMin, latencyMax]); } - if (!throughputUserModified.current) { - setThroughputRange([metricRanges.throughputMin, metricRanges.throughputMax]); + if ( + !throughputUserModified.current && + Number.isFinite(throughputMin) && + Number.isFinite(throughputMax) + ) { + setThroughputRange([throughputMin, throughputMax]); } }, [metricRanges, metricsLoading, metricsMap]); Suggestion importance[1-10]: 7 __ Why: Valid concern: metricRanges can produce Infinity when no matching metrics exist, which would break the EuiDualRange slider. Adding a Number.isFinite guard is a reasonable defensive improvement.	Medium
General	Restore top-level error handling The try/catch around Promise.allSettled was removed but promqlService.executeInstantQuery can still throw synchronously (e.g., during argument validation), or the params.endTime.getTime() call could throw if endTime is invalid. An unhandled rejection here would leak past the effect. Wrap the async body in try/catch to preserve error handling and always reset isLoading. public/components/apm/shared/hooks/use_dependency_metrics.ts [73-78] const fetchMetrics = async () => { setIsLoading(true); setError(null); - const timeRangeDuration = calculateTimeRangeDuration(params.startTime, params.endTime); - const time = Math.floor(params.endTime.getTime() / 1000); + try { + const timeRangeDuration = calculateTimeRangeDuration(params.startTime, params.endTime); + const time = Math.floor(params.endTime.getTime() / 1000); + // ... existing settled logic ... + } catch (err) { + if (abortController.signal.aborted) return; + console.error('[useDependencyMetrics] Unexpected error:', err); + setError(err instanceof Error ? err : new Error('Unknown error')); + } finally { + if (!abortController.signal.aborted) setIsLoading(false); + } +}; Suggestion importance[1-10]: 5 __ Why: Reasonable point: without a try/catch, unexpected synchronous errors (or non-settled paths) would leave isLoading stuck true. Adding error handling improves robustness, though the risk is relatively low.	Low
	Memoize derived sparkline values sparklineNames, sparklineKey, and sparklineFilter are computed on every render without memoization, producing new array/string references each render. Since sparklineKey is a dependency of the sparkline useEffect, this is fine, but the unmemoized sparklineNames.length check inside the effect condition and the array recreation can cause subtle inconsistencies. Wrap these in useMemo keyed on params.sparklineServices for stability. public/components/apm/shared/hooks/use_services_red_metrics.ts [142-148] -// Unique service names on the visible page -> a bounded `service=~"..."` -// filter for the sparkline range queries. Small (<=page size), so it stays -// well under the 10,000-char PromQL limit. sparklineKey drives refetch. -const sparklineNames = Array.from( - new Set((params.sparklineServices ?? []).map((s) => s.serviceName)) +const sparklineNames = useMemo( + () => Array.from(new Set((params.sparklineServices ?? []).map((s) => s.serviceName))), + [params.sparklineServices] ); -const sparklineKey = sparklineNames.join('|'); -const sparklineFilter = sparklineNames.length - ? `service=~"${sparklineNames.map(escapePromQLRegex).join('|')}"` - : ''; +const sparklineKey = useMemo(() => sparklineNames.join('|'), [sparklineNames]); +const sparklineFilter = useMemo( + () => + sparklineNames.length + ? `service=~"${sparklineNames.map(escapePromQLRegex).join('|')}"` + : '', + [sparklineNames] +); Suggestion importance[1-10]: 4 __ Why: Memoizing these values would provide minor stability improvements, but since sparklineKey is a string used as effect dependency and behaves like a value equality check, the practical impact is limited.	Low
	Sanitize service filter input When serviceFilter is an empty string, filter(Boolean) correctly drops it. However, when a caller passes a non-empty filter like service="a|b", the selector becomes {service="a|b", remoteService="", namespace="span_derived"} — correct. But when the filter is provided with a leading/trailing comma or whitespace (as some callers historically did), the selector would be malformed. Consider trimming the input to be defensive. public/components/apm/query_services/query_requests/promql_queries.ts [37-38] const buildServicesNodeSelector = (serviceFilter: string): string => - [serviceFilter, 'remoteService=""', 'namespace="span_derived"'].filter(Boolean).join(', '); + [serviceFilter.trim().replace(/^,|,$/g, ''), 'remoteService=""', 'namespace="span_derived"'] + .filter(Boolean) + .join(', '); Suggestion importance[1-10]: 2 __ Why: The suggestion is speculative; since serviceFilter in this PR is always an empty string or well-formed, adding trim/comma-cleaning provides marginal defensive value.	Low

Suggestions up to commit 6f8c14c

Category	Suggestion	Impact
Possible issue	Use composite key in combined metrics loop The combined metrics map is being keyed by serviceName alone here, but latencyMap, throughputFailureMap, totalCountMap, and failureRatioInstantMap are now keyed by serviceNodeKey(serviceName, environment). All lookups in this loop will miss, leaving the catalog without metrics. Iterate over params.services and use serviceNodeKey for both lookups and storage. public/components/apm/shared/hooks/use_services_red_metrics.ts [357-364] -const avgFailureRatio = failureRatioInstantMap.has(serviceName) - ? failureRatioInstantMap.get(serviceName)! +const key = serviceNodeKey(serviceName, environment); +const avgFailureRatio = failureRatioInstantMap.has(key) + ? failureRatioInstantMap.get(key)! : failureData.length > 0 ? failureData.reduce((sum, point) => sum + point.value, 0) / failureData.length : 0; -combined.set(serviceName, { +combined.set(key, { Suggestion importance[1-10]: 8 __ Why: This appears to be a real bug: the storage maps were changed to use serviceNodeKey(serviceName, environment) but the combined-metrics loop still uses serviceName alone for lookups and storage, which would cause metrics to be missing. However, the full diff context is truncated so this cannot be fully verified.	Medium
	Refs in useMemo do not trigger recomputation Reading latencyUserModified.current and throughputUserModified.current inside a useMemo will not retrigger memoization when the ref mutates, so toggling the slider will not cause the filter to re-apply until another dependency changes. Track the "user modified" state in useState (or include a state-backed version bump in the deps) so the memo actually reruns when the filter becomes active. public/components/apm/pages/services_home/services_home.tsx [677-686] +// Filter by latency range (only if range has been adjusted from full range) +const isLatencyFilterActive = latencyUserModified.current; +if (isLatencyFilterActive) { + filtered = filtered.filter((service) => { - Suggestion importance[1-10]: 6 __ Why: Valid concern that refs in useMemo won't trigger recomputation on mutation, but the sliders likely update latencyRange/throughputRange state alongside the ref, which are already in the deps list and would trigger the memo. The improved_code is identical to existing_code, providing no actual fix.	Low
General	Verify empty-label match for SERVER spans Adding remoteService="" to the application-level aggregates is a semantic change: series produced without the remoteService label at all will no longer match (in Prometheus, label="" only matches series where the label is absent OR empty depending on ingestion). Verify that Data Prepper's SERVER-span metrics actually emit remoteService="" rather than omitting the label; if some emitters omit it, the root topology node will silently show zero after this change. public/components/apm/query_services/query_requests/promql_queries.ts [775] +export const getQueryApplicationRequests = (): string => ` +sum(request{remoteService="",namespace="span_derived"}) +`; - Suggestion importance[1-10]: 4 __ Why: The suggestion only asks to verify a behavior, and improved_code is identical to existing_code. It raises a valid concern about label matching semantics but provides no actual code change.	Low
Security	Escape backslashes before quotes for safety The backslash replacement is applied in a single pass with a global regex, so a literal backslash in input becomes \ and then a following quote becomes ' — but the order of matches means a backslash immediately preceding a quote may be double-escaped incorrectly, or worse, an input ending in </code> could produce a dangling escape that turns the following closing ' in the query into an escaped quote. Escape backslashes first, then quotes, to guarantee correct sequencing. public/components/apm/query_services/query_requests/escape_utils.ts [22] /** Escape a value for use inside a single-quoted PPL string literal. */ export const escapePPLString = (value: string): string => - value.replace(/[\\']/g, (ch) => `\\${ch}`); + value.replace(/\\/g, '\\\\').replace(/'/g, "\\'"); Suggestion importance[1-10]: 3 __ Why: The original single-pass regex with a character class [\\'] and the callback \\${ch} actually escapes each matched character correctly and independently, so backslashes and quotes are both properly escaped without the sequencing issue described. The suggestion is based on a misunderstanding of the existing code.	Low

Suggestions up to commit b239601

Category	Suggestion	Impact
Possible issue	Fix mismatched lookup key in combined map The combined metrics map is keyed by serviceName here, but throughputFailureMap, totalCountMap, latencyInstantMap, and failureRatioInstantMap are now keyed by serviceNodeKey(serviceName, environment). All the .get(serviceName) lookups will miss and produce zero metrics for every row. Iterate over params.services and use serviceNodeKey(serviceName, environment) consistently as both the lookup key and the map key. public/components/apm/shared/hooks/use_services_red_metrics.ts [351-364] -const avgLatency = latencyInstantMap.get(serviceName) || 0; -// Use sum_over_time total / time range for accurate req/s -// (plain gauge range query is inflated by Prometheus stale lookback) -const timeRangeSeconds = endTimeSec - startTimeSec; -const totalRequests = totalCountMap.get(serviceName) || 0; -const avgThroughput = timeRangeSeconds > 0 ? totalRequests / timeRangeSeconds : 0; -// Ratio-of-sums over the window (unbiased); fall back to the sparkline mean. -const avgFailureRatio = failureRatioInstantMap.has(serviceName) - ? failureRatioInstantMap.get(serviceName)! - : failureData.length > 0 - ? failureData.reduce((sum, point) => sum + point.value, 0) / failureData.length - : 0; +params.services.forEach(({ serviceName, environment }) => { + const key = serviceNodeKey(serviceName, environment); + const latencyData = latencyMap.get(key) || []; + const tf = throughputFailureMap.get(key); + const failureData = tf?.failureRatio || []; + const avgLatency = latencyInstantMap.get(key) || 0; + const timeRangeSeconds = endTimeSec - startTimeSec; + const totalRequests = totalCountMap.get(key) || 0; + const avgThroughput = timeRangeSeconds > 0 ? totalRequests / timeRangeSeconds : 0; + const avgFailureRatio = failureRatioInstantMap.has(key) + ? failureRatioInstantMap.get(key)! + : failureData.length > 0 + ? failureData.reduce((sum, point) => sum + point.value, 0) / failureData.length + : 0; + combined.set(key, { -combined.set(serviceName, { - Suggestion importance[1-10]: 9 __ Why: This appears to be a critical bug: the maps are now keyed by serviceNodeKey(serviceName, environment) but the combined map lookups still use serviceName alone, which would cause all metric lookups to miss and produce zero metrics. However, the full context of the loop isn't visible in the diff, so verification is needed.	High
	Avoid silent truncation on single-service lookup getQueryGetService is intended to fetch a single service's attributes, but it applies head 1000 without any sort. When the service has many connection rows, this can silently truncate before returning the target service's data. Either add a stable sort (e.g., by timestamp desc) before head, or use head 1 since dedup+fields make one row sufficient. public/components/apm/query_services/query_requests/ppl_queries.ts [132] export function getQueryGetService( queryIndex: string, startTime?: string | Date, endTime?: string | Date, environment?: string, serviceName?: string ): string { let query = `source=${queryIndex}`; query += buildTimeFilterClause(startTime, endTime); // Filter by service keyAttributes if provided if (environment) { query += ` | where sourceNode.keyAttributes.environment = '${escapePPLString(environment)}'`; } if (serviceName) { query += ` | where sourceNode.keyAttributes.name = '${escapePPLString(serviceName)}'`; } query += ` | dedup nodeConnectionHash`; query += ` | fields sourceNode.keyAttributes, sourceNode.groupByAttributes`; - query += ` | head ${DEFAULT_ROW_LIMIT}`; + query += ` | head 1`; return query; } Suggestion importance[1-10]: 6 __ Why: Valid concern: getQueryGetService applies head 1000 without a sort, which could truncate before returning the target service's data on large datasets. Using head 1 after dedup would be more appropriate for a single-service lookup, though the impact depends on how the function is actually used.	Low
General	Verify scope after removing service filter Removing the service filter means the PromQL now aggregates across every service in namespace="span_derived", not just the ones in params.services. On large clusters this can return substantially more series than needed and, for cross-tenant / multi-app dashboards, may leak metrics for services outside the current topology view. Consider grouping by (environment, service) (as done in use_services_red_metrics) and filtering client-side, and/or verifying the extra series volume is acceptable. public/components/apm/shared/hooks/use_service_map_metrics.ts [77] -// Rely on `sum by (service)` grouping rather than a service=~"..." filter, -// which grew past the 10,000-char PromQL limit on large topologies. +// Rely on `sum by (environment, service)` grouping rather than a +// service=~"..." filter, which grew past the 10,000-char PromQL limit. +// Note: this returns every service in namespace="span_derived"; results +// are filtered client-side to params.services. const serviceFilter = ''; Suggestion importance[1-10]: 5 __ Why: Legitimate concern about the tradeoff: removing the service filter aggregates across all services in the namespace, potentially increasing response size and exposing metrics beyond the current scope. The suggestion is mostly a verification request with a minor comment improvement.	Low
	Escape newlines in PromQL label values PromQL string literals also treat newline (\n) as a control character that terminates/invalidates the string. A service or environment name containing a newline (or other control character) will break the query even with this escaper. Include \n (and ideally \r) in the escape set. public/components/apm/query_services/query_requests/escape_utils.ts [17-18] /** Escape a value for use inside a PromQL exact-match label (label="..."). */ export const escapePromQLLabel = (value: string): string => - value.replace(/[\\"]/g, (ch) => `\\${ch}`); + value + .replace(/[\\"]/g, (ch) => `\\${ch}`) + .replace(/\n/g, '\\n') + .replace(/\r/g, '\\r'); Suggestion importance[1-10]: 4 __ Why: Valid edge case: PromQL string literals don't allow raw newlines, so escaping them would harden the escape function. However, service/environment names containing newlines are extremely unlikely in practice, so the impact is minor.	Low

Suggestions up to commit 8bfc76a

Category	Suggestion	Impact
Possible issue	Fix inconsistent map key between producer and consumer The combined map in useServicesRedMetrics is still built with plain serviceName as the key (via combined.set(serviceName, ...)), but callers now look it up using serviceNodeKey(serviceName, environment). This mismatch means metricsMap.get(...) will always miss, so all displayed metrics, filters, and sorts return 0. Update the combined.set and the corresponding latencyMap/throughputFailureMap/etc. read keys to use serviceNodeKey. public/components/apm/shared/hooks/use_services_red_metrics.ts [351-364] +fullyFilteredItems.forEach((service) => { + const metrics = metricsMap.get(serviceNodeKey(service.serviceName, service.environment)); + if (!metrics) return; - Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical bug: the combined map is populated with serviceName as key while consumers (e.g., services_home.tsx) now look up using serviceNodeKey(serviceName, environment). This would cause all metric lookups to miss.	High
	Use composite key when building combined metrics The combined-metrics loop iterates by serviceName but the new maps (latencyInstantMap, totalCountMap, failureRatioInstantMap, latencyMap, throughputFailureMap) are now keyed by serviceNodeKey(serviceName, environment). As written, every .get(serviceName) returns undefined, collapsing latency/throughput/failure to zero for every service. Iterate over the composite keys (or reconstruct them from params.services) and set the combined map with the same composite key. public/components/apm/shared/hooks/use_services_red_metrics.ts [351-361] - // Use instant query result for true percentile over full time range - const avgLatency = latencyInstantMap.get(serviceName) || 0; - // Use sum_over_time total / time range for accurate req/s - // (plain gauge range query is inflated by Prometheus stale lookback) + const key = serviceNodeKey(serviceName, environment); + const avgLatency = latencyInstantMap.get(key) || 0; const timeRangeSeconds = endTimeSec - startTimeSec; - const totalRequests = totalCountMap.get(serviceName) || 0; + const totalRequests = totalCountMap.get(key) || 0; const avgThroughput = timeRangeSeconds > 0 ? totalRequests / timeRangeSeconds : 0; - // Ratio-of-sums over the window (unbiased); fall back to the sparkline mean. - const avgFailureRatio = failureRatioInstantMap.has(serviceName) - ? failureRatioInstantMap.get(serviceName)! + const avgFailureRatio = failureRatioInstantMap.has(key) + ? failureRatioInstantMap.get(key)! : failureData.length > 0 Suggestion importance[1-10]: 9 __ Why: This overlaps with suggestion 1 and correctly identifies that inside the combined metrics loop, all .get(serviceName) calls should use the composite key since the source maps are now keyed by serviceNodeKey. This is a critical correctness issue.	High
General	Verify application-root aggregation semantics change Adding remoteService="" to the application-level aggregate queries (requests, faults, errors, latency) changes their semantics: they now count only SERVER-span metrics, whereas the previous flyout showed all traffic including CLIENT-span calls. If the "Application root" node is meant to represent total application activity this is a silent behavior change; confirm the intent and, if needed, keep the old application-wide aggregation for the root node while using the SERVER-only variant elsewhere. public/components/apm/query_services/query_requests/promql_queries.ts [763] +export const getQueryApplicationRequests = (): string => ` +sum(request{remoteService="",namespace="span_derived"}) +`; - Suggestion importance[1-10]: 6 __ Why: Reasonable observation about semantic change from adding remoteService="" filter to application-level queries, but it's primarily a verification request and the improved_code is identical to existing_code.	Low
	Surface truncation from row-limited PPL queries Appending | head 1000 after dedup silently truncates topology/service-map results to the first 1000 rows without any user-visible signal that data was dropped. On large fleets this yields an incomplete graph or dependency table that looks authoritative. Consider fetching LIMIT+1 and surfacing a "results truncated" flag/notice to the UI so users know to narrow the time range or filter further. public/components/apm/query_services/query_requests/ppl_queries.ts [88] query += ` | dedup nodeConnectionHash`; query += ` | fields sourceNode.keyAttributes, sourceNode.groupByAttributes, targetNode.keyAttributes, targetNode.groupByAttributes`; -+ query += ` | head ${DEFAULT_ROW_LIMIT}`; + query += ` | head ${DEFAULT_ROW_LIMIT + 1}`; return query; -} Suggestion importance[1-10]: 5 __ Why: Valid UX concern about silent truncation of results at 1000 rows. However, the improved code only adds +1 without wiring the actual detection/UI signal, making the suggestion incomplete.	Low

[github-actions]

Persistent review updated to latest commit 8bfc76a

[github-actions]

Persistent review updated to latest commit b239601

[github-actions]

Persistent review updated to latest commit 6f8c14c

[github-actions]

Persistent review updated to latest commit a7b10e5

[github-actions]

Persistent review updated to latest commit 49034aa

[github-actions]

Persistent review updated to latest commit 9a71e59