-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[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
Conversation
Signed-off-by: abrar <[email protected]>
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.
Code Review
This pull request extends the dashboard functionality by adding support for more panel types and customization options, notably the 'table' panel. The changes introduce more flexibility in generating Grafana dashboards.
My review focuses on improving the maintainability and clarity of the new code:
- In
grafana_dashboard_factory.py, I've suggested refactoring a large block of conditional logic to make it more readable and easier to maintain by using constant sets for panel types. - In
common.py, I've pointed out a confusing use of a duplicated ID in a new panel template and a misleadingly named parameter (heatmap_yaxis_label) that should be renamed for clarity. - In
test_metrics_head.py, I've suggested a small refactoring to remove duplicated code in a test case.
Overall, the changes are a good addition, and the suggested improvements will help with future development.
| "overrides": [], | ||
| }, | ||
| "gridPos": {"h": 8, "w": 12, "x": 0, "y": 0}, | ||
| "id": 26, |
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 id for this TABLE_PANEL_TEMPLATE is 26. This ID is also used by GRAPH_PANEL_TEMPLATE, PIE_CHART_TEMPLATE, and BAR_CHART_PANEL_TEMPLATE. While the panel id is 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.
| # 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" | ||
|
|
||
| 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 |
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.
This large block of conditional logic has many repeated checks like panel.template in (...). This reduces readability and makes it harder to maintain.
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.
| 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"] |
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 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"]| 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 comment
The reason will be displayed to describe this comment to others. Learn more.
BAR_CHART panels ignore stack and linewidth settings
High Severity
The stack, linewidth, and nullPointMode settings are only applied when panel.template == PanelTemplate.GRAPH, but BAR_CHART panels also have these fields and need them applied. The BAR_CHART_PANEL_TEMPLATE has "stack": True and "linewidth": 1 as defaults, but several existing BAR_CHART panels in data_dashboard_panels.py explicitly set stack=False. Since the condition excludes BAR_CHART, these panels will incorrectly render as stacked when they should be grouped (unstacked). The condition should include PanelTemplate.BAR_CHART for stack, linewidth, and nullPointMode.
🔬 Verification Test
Test 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:
cd /workspace && python test_bar_chart_stack_bug.py
Output:
BAR_CHART_PANEL_TEMPLATE stack default: True
Panel.stack setting: False
Generated template stack value: True
BUG: stack=False was NOT applied: True
Why this proves the bug: The panel was created with stack=False, but the generated template has stack: True (the default from BAR_CHART_PANEL_TEMPLATE). This proves the stack setting is being ignored for BAR_CHART panels, which will cause existing data dashboard histogram panels to render incorrectly as stacked instead of grouped.
Signed-off-by: abrar <[email protected]>
Signed-off-by: abrar <[email protected]> Signed-off-by: jasonwrwang <[email protected]>
No description provided.