-
Notifications
You must be signed in to change notification settings - Fork 7.2k
[Dashboard] Support more panels in dashboard #60018
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| from ray.dashboard.modules.metrics.dashboards.common import DashboardConfig | ||
|
|
||
|
|
||
| def get_serve_dashboard_config() -> DashboardConfig: | ||
| from ray.dashboard.modules.metrics.dashboards.serve_dashboard_panels import ( | ||
| serve_dashboard_config, | ||
| ) | ||
|
|
||
| return serve_dashboard_config | ||
|
|
||
|
|
||
| # Anyscale overrides |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,16 +9,14 @@ | |
| from ray.dashboard.modules.metrics.dashboards.common import ( | ||
| DashboardConfig, | ||
| Panel, | ||
| PanelTemplate, | ||
| ) | ||
| from ray.dashboard.modules.metrics.dashboards.data_dashboard_panels import ( | ||
| data_dashboard_config, | ||
| ) | ||
| from ray.dashboard.modules.metrics.dashboards.default_dashboard_panels import ( | ||
| default_dashboard_config, | ||
| ) | ||
| from ray.dashboard.modules.metrics.dashboards.serve_dashboard_panels import ( | ||
| serve_dashboard_config, | ||
| ) | ||
| from ray.dashboard.modules.metrics.dashboards.serve_deployment_dashboard_panels import ( | ||
| serve_deployment_dashboard_config, | ||
| ) | ||
|
|
@@ -28,6 +26,7 @@ | |
| from ray.dashboard.modules.metrics.dashboards.train_dashboard_panels import ( | ||
| train_dashboard_config, | ||
| ) | ||
| from ray.dashboard.modules.metrics.default_impl import get_serve_dashboard_config | ||
|
|
||
| GRAFANA_DASHBOARD_UID_OVERRIDE_ENV_VAR_TEMPLATE = "RAY_GRAFANA_{name}_DASHBOARD_UID" | ||
| GRAFANA_DASHBOARD_GLOBAL_FILTERS_OVERRIDE_ENV_VAR_TEMPLATE = ( | ||
|
|
@@ -96,7 +95,7 @@ def generate_serve_grafana_dashboard() -> Tuple[str, str]: | |
| Returns: | ||
| Tuple with format content, uid | ||
| """ | ||
| return _generate_grafana_dashboard(serve_dashboard_config) | ||
| return _generate_grafana_dashboard(get_serve_dashboard_config()) | ||
|
|
||
|
|
||
| def generate_serve_deployment_grafana_dashboard() -> Tuple[str, str]: | ||
|
|
@@ -224,17 +223,117 @@ def _generate_panel_template( | |
| "y": base_y_position + (row_number * PANEL_HEIGHT), | ||
| } | ||
|
|
||
| template["yaxes"][0]["format"] = panel.unit | ||
| template["fill"] = panel.fill | ||
| template["stack"] = panel.stack | ||
| template["linewidth"] = panel.linewidth | ||
| # Set unit format for legacy graph-style panels (GRAPH, HEATMAP, STAT, GAUGE, PIE_CHART, BAR_CHART) | ||
| if panel.template in ( | ||
| PanelTemplate.GRAPH, | ||
| PanelTemplate.HEATMAP, | ||
| PanelTemplate.STAT, | ||
| PanelTemplate.GAUGE, | ||
| PanelTemplate.PIE_CHART, | ||
| PanelTemplate.BAR_CHART, | ||
| ): | ||
| template["yaxes"][0]["format"] = panel.unit | ||
|
|
||
| # Set fieldConfig unit (for newer panel types with fieldConfig.defaults) | ||
| if panel.template in ( | ||
| PanelTemplate.STAT, | ||
| PanelTemplate.GAUGE, | ||
| PanelTemplate.HEATMAP, | ||
| PanelTemplate.PIE_CHART, | ||
| PanelTemplate.BAR_CHART, | ||
| PanelTemplate.TABLE, | ||
| PanelTemplate.GRAPH, | ||
| ): | ||
| template["fieldConfig"]["defaults"]["unit"] = panel.unit | ||
|
|
||
| # Set fill, stack, linewidth, nullPointMode (only for GRAPH panels) | ||
| if panel.template == PanelTemplate.GRAPH: | ||
| template["fill"] = panel.fill | ||
| template["stack"] = panel.stack | ||
| template["linewidth"] = panel.linewidth | ||
| if panel.stack is True: | ||
| template["nullPointMode"] = "connected" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BAR_CHART panels ignore stack and linewidth settingsHigh Severity The 🔬 Verification TestTest code: # test_bar_chart_stack_bug.py
import copy
from ray.dashboard.modules.metrics.dashboards.common import (
Panel, Target, PanelTemplate, BAR_CHART_PANEL_TEMPLATE
)
from ray.dashboard.modules.metrics.grafana_dashboard_factory import _generate_panel_template
# Create a BAR_CHART panel with stack=False (like the data dashboard panels)
panel = Panel(
title="Test",
description="Test",
id=1,
unit="short",
targets=[Target(expr="test", legend="test")],
stack=False, # Explicitly setting stack to False
template=PanelTemplate.BAR_CHART,
)
# Generate the template
result = _generate_panel_template(panel, [], 0, 0)
print(f"BAR_CHART_PANEL_TEMPLATE stack default: {BAR_CHART_PANEL_TEMPLATE['stack']}")
print(f"Panel.stack setting: {panel.stack}")
print(f"Generated template stack value: {result['stack']}")
print(f"BUG: stack=False was NOT applied: {result['stack'] == True}")Command run: Output: Why this proves the bug: The panel was created with |
||
|
|
||
| if panel.hideXAxis: | ||
| template.setdefault("xaxis", {})["show"] = False | ||
|
|
||
| # Handle stacking visualization | ||
| if panel.stack is True: | ||
| template["nullPointMode"] = "connected" | ||
| # Handle optional panel customization fields | ||
|
|
||
| # Thresholds (for panels with fieldConfig.defaults.thresholds) | ||
| if panel.thresholds is not None: | ||
| if panel.template in (PanelTemplate.STAT, PanelTemplate.GAUGE): | ||
| template["fieldConfig"]["defaults"]["thresholds"][ | ||
| "steps" | ||
| ] = panel.thresholds | ||
|
|
||
| # Value mappings (for panels with fieldConfig.defaults.mappings) | ||
| if panel.value_mappings is not None: | ||
| if panel.template in ( | ||
| PanelTemplate.STAT, | ||
| PanelTemplate.GAUGE, | ||
| PanelTemplate.TABLE, | ||
| ): | ||
| template["fieldConfig"]["defaults"]["mappings"] = panel.value_mappings | ||
|
|
||
| # Color mode (for STAT panels with options.colorMode) | ||
| if panel.color_mode is not None: | ||
| if panel.template == PanelTemplate.STAT: | ||
| template["options"]["colorMode"] = panel.color_mode | ||
|
|
||
| # Legend mode | ||
| if panel.legend_mode is not None: | ||
| if panel.template in (PanelTemplate.GRAPH, PanelTemplate.BAR_CHART): | ||
| # For graph panels (legacy format with top-level legend object) | ||
| template["legend"]["show"] = panel.legend_mode != "hidden" | ||
| template["legend"]["alignAsTable"] = panel.legend_mode == "table" | ||
| elif panel.template == PanelTemplate.PIE_CHART: | ||
| # For PIE_CHART (options.legend.displayMode) | ||
| template["options"]["legend"]["displayMode"] = panel.legend_mode | ||
|
|
||
| # Min/max values (for panels with fieldConfig.defaults) | ||
| if panel.min_val is not None or panel.max_val is not None: | ||
| if panel.template in ( | ||
| PanelTemplate.STAT, | ||
| PanelTemplate.GAUGE, | ||
| PanelTemplate.HEATMAP, | ||
| PanelTemplate.PIE_CHART, | ||
| PanelTemplate.BAR_CHART, | ||
| PanelTemplate.TABLE, | ||
| PanelTemplate.GRAPH, | ||
| ): | ||
| if panel.min_val is not None: | ||
| template["fieldConfig"]["defaults"]["min"] = panel.min_val | ||
| if panel.max_val is not None: | ||
| template["fieldConfig"]["defaults"]["max"] = panel.max_val | ||
|
|
||
| # Reduce calculation (for panels with options.reduceOptions) | ||
| if panel.reduce_calc is not None: | ||
| if panel.template in ( | ||
| PanelTemplate.STAT, | ||
| PanelTemplate.GAUGE, | ||
| PanelTemplate.PIE_CHART, | ||
| ): | ||
| template["options"]["reduceOptions"]["calcs"] = [panel.reduce_calc] | ||
|
|
||
| # Handle heatmap-specific options | ||
| if panel.heatmap_color_scheme is not None: | ||
| if panel.template == PanelTemplate.HEATMAP: | ||
| template["options"]["color"]["scheme"] = panel.heatmap_color_scheme | ||
|
|
||
| if panel.heatmap_color_reverse is not None: | ||
| if panel.template == PanelTemplate.HEATMAP: | ||
| template["options"]["color"]["reverse"] = panel.heatmap_color_reverse | ||
|
|
||
| if panel.heatmap_yaxis_label is not None: | ||
| if panel.template in ( | ||
| PanelTemplate.GRAPH, | ||
| PanelTemplate.HEATMAP, | ||
| PanelTemplate.STAT, | ||
| PanelTemplate.GAUGE, | ||
| PanelTemplate.PIE_CHART, | ||
| PanelTemplate.BAR_CHART, | ||
| ): | ||
| template["yaxes"][0]["label"] = panel.heatmap_yaxis_label | ||
|
|
||
| return template | ||
|
Comment on lines
+226
to
338
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This large block of conditional logic has many repeated checks like To improve this, you could define constant sets for groups of panel templates that share configuration options at the top of the file. For example: PANELS_WITH_YAXES = {
PanelTemplate.GRAPH,
PanelTemplate.HEATMAP,
PanelTemplate.STAT,
PanelTemplate.GAUGE,
PanelTemplate.PIE_CHART,
PanelTemplate.BAR_CHART,
}Then, you can simplify the conditions: if panel.template in PANELS_WITH_YAXES:
template["yaxes"][0]["format"] = panel.unitApplying this pattern throughout this function will make the code more declarative and easier to manage. |
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,8 +181,14 @@ def test_metrics_folder_with_dashboard_override( | |
| contents = json.loads(f.read()) | ||
| assert contents["uid"] == serve_uid | ||
| for panel in contents["panels"]: | ||
| for target in panel["targets"]: | ||
| assert serve_global_filters in target["expr"] | ||
| if panel["type"] == "row": | ||
| # Row panels contain nested panels, not targets directly | ||
| for nested_panel in panel.get("panels", []): | ||
| for target in nested_panel["targets"]: | ||
| assert serve_global_filters in target["expr"] | ||
| else: | ||
| for target in panel["targets"]: | ||
| assert serve_global_filters in target["expr"] | ||
|
Comment on lines
183
to
+191
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic to check targets is duplicated for row panels and other panels. You can refactor this to avoid repetition and improve readability. panels_to_check = []
if panel["type"] == "row":
# Row panels contain nested panels, not targets directly
panels_to_check.extend(panel.get("panels", []))
else:
panels_to_check.append(panel)
for p in panels_to_check:
for target in p.get("targets", []):
assert serve_global_filters in target["expr"] |
||
| for variable in contents["templating"]["list"]: | ||
| if variable["name"] == "datasource": | ||
| continue | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
idfor thisTABLE_PANEL_TEMPLATEis26. This ID is also used byGRAPH_PANEL_TEMPLATE,PIE_CHART_TEMPLATE, andBAR_CHART_PANEL_TEMPLATE. While the panelidis overridden during dashboard generation, using duplicate IDs in templates is confusing and could lead to subtle bugs if the generation logic changes. Please consider using a unique placeholder ID for each panel template to avoid potential conflicts.