Skip to content

fix: prevent native histogram double-rebuild race condition#1113

Open
nicwestvold wants to merge 4 commits intomainfrom
fix/cannot-load-metrics-drilldown
Open

fix: prevent native histogram double-rebuild race condition#1113
nicwestvold wants to merge 4 commits intomainfrom
fix/cannot-load-metrics-drilldown

Conversation

@nicwestvold
Copy link
Contributor

@nicwestvold nicwestvold commented Mar 2, 2026

✨ Description

Native histogram metrics could fail to load: subscribeToStateChanges set panelConfig.type = 'heatmap' but omitted metricType: 'native-histogram', so queries never updated. A secondary race allowed both detection paths to independently trigger buildVizPanel().

📖 Changes

  • Set metricType: 'native-histogram' in the subscribeToStateChanges path
  • Fix stale closure: read this.state.panelConfig at resolution time
  • Skip state update if the other path already applied heatmap type
  • Unsubscribe on first Done event unconditionally
  • Replace ! assertions in query builders with guards that warn and skip

🧪 How to test?

  • Open a native histogram metric; verify it renders as a heatmap with native histogram queries
  • Automated: new unit tests in GmdVizPanel.test.ts and the three query builder test files

@nicwestvold nicwestvold self-assigned this Mar 2, 2026
Copilot AI review requested due to automatic review settings March 2, 2026 18:33
@nicwestvold nicwestvold requested a review from a team as a code owner March 2, 2026 18:33
@github-actions github-actions bot added the fix label Mar 2, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a race in GmdVizPanel where two async detection paths (metadata vs. returned dataframe type) could both trigger a rebuild when a metric is a native histogram, and adds defensive handling in query builders so persisted configs referencing unknown PromQL functions don’t crash query construction.

Changes:

  • Add guards in GmdVizPanel to prevent redundant “switch-to-heatmap” rebuilds and unsubscribe deterministically on first LoadingState.Done.
  • Add runtime checks around PROMQL_FUNCTIONS.get(fn) in timeseries/stat/percentiles query param builders, logging and skipping unknown functions.
  • Add/extend unit tests for the histogram race guard paths and unknown-function handling.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/shared/GmdVizPanel/GmdVizPanel.tsx Adds guards + earlier unsubscribe to prevent double-rebuild when native histogram is detected via two async paths.
src/shared/GmdVizPanel/tests/GmdVizPanel.test.ts New tests covering the two-path guard behavior and unsubscribe behavior.
src/shared/GmdVizPanel/types/timeseries/getTimeseriesQueryRunnerParams.ts Skips unknown PromQL function entries and logs a warning instead of crashing.
src/shared/GmdVizPanel/types/timeseries/tests/getTimeseriesQueryRunnerParams.test.ts Adds test asserting unknown PromQL functions are skipped and warning is logged.
src/shared/GmdVizPanel/types/stat/getStatQueryRunnerParams.ts Same unknown-function guard + warning for stat query builder.
src/shared/GmdVizPanel/types/stat/tests/getStatQueryRunnerParams.test.ts Adds unknown-function test coverage for stat query builder.
src/shared/GmdVizPanel/types/percentiles/getPercentilesQueryRunnerParams.ts Adds unknown-function guards for both histogram and non-histogram percentile query paths.
src/shared/GmdVizPanel/types/percentiles/tests/getPercentilesQueryRunnerParams.test.ts Adds tests for unknown-function handling in both percentile query paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants