Skip to content

Commit

Permalink
Add unit rule smart exceptions
Browse files Browse the repository at this point in the history
  • Loading branch information
v-zhuravlev committed Nov 5, 2024
1 parent a9d6c25 commit 4b54416
Show file tree
Hide file tree
Showing 6 changed files with 222 additions and 59 deletions.
8 changes: 6 additions & 2 deletions docs/rules/panel-units-rule.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ Checks that every panel has a unit specified, and that the unit is valid per the
It currently only checks panels of type ["stat", "singlestat", "graph", "table", "timeseries", "gauge"].

# Best Practice
All panels should have all of their axis labeled with an apprioriate unit.
All panels should have an apprioriate unit set.

# Possible exceptions
A panel may be visualizing something which does not have a predefined unit, or which is self explanatory from the vizualization title. In this case you may wish to create a lint exclusion for this rule.
This rule is automatically excluded when:
- Value mappings are set in a panel.
- A Stat panel is configured to show non-numeric values (like label's value), for that 'Fields options' are configured to any value other than 'Numeric fields' (which is default).

Also, a panel may be visualizing something which does not have a predefined unit, or which is self explanatory from the vizualization title. In this case you may wish to create a lint exclusion for this rule.
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ require (
github.com/gorilla/mux v1.8.1 // indirect
github.com/grafana/dskit v0.0.0-20240905221822-931a021fb06b // indirect
github.com/grafana/gomemcache v0.0.0-20240229205252-cd6a66d6fb56 // indirect
github.com/grafana/grafana-foundation-sdk/go v0.0.0-20241101005901-83e3491f2a70 // indirect
github.com/grafana/jsonparser v0.0.0-20240425183733-ea80629e1a32 // indirect
github.com/grafana/loki/pkg/push v0.0.0-20231124142027-e52380921608 // indirect
github.com/grafana/pyroscope-go/godeltaprof v0.1.8 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,8 @@ github.com/grafana/dskit v0.0.0-20240905221822-931a021fb06b h1:x2HCzk29I0o5pRPfq
github.com/grafana/dskit v0.0.0-20240905221822-931a021fb06b/go.mod h1:SPLNCARd4xdjCkue0O6hvuoveuS1dGJjDnfxYe405YQ=
github.com/grafana/gomemcache v0.0.0-20240229205252-cd6a66d6fb56 h1:X8IKQ0wu40wpvYcKfBcc5T4QnhdQjUhtUtB/1CY89lE=
github.com/grafana/gomemcache v0.0.0-20240229205252-cd6a66d6fb56/go.mod h1:PGk3RjYHpxMM8HFPhKKo+vve3DdlPUELZLSDEFehPuU=
github.com/grafana/grafana-foundation-sdk/go v0.0.0-20241101005901-83e3491f2a70 h1:69GI3KsF851YnwYp6zHdsskcGp3ZnGsWc+ve8vMp1mc=
github.com/grafana/grafana-foundation-sdk/go v0.0.0-20241101005901-83e3491f2a70/go.mod h1:WtWosval1KCZP9BGa42b8aVoJmVXSg0EvQXi9LDSVZQ=
github.com/grafana/jsonparser v0.0.0-20240425183733-ea80629e1a32 h1:NznuPwItog+rwdVg8hAuGKP29ndRSzJAwhxKldkP8oQ=
github.com/grafana/jsonparser v0.0.0-20240425183733-ea80629e1a32/go.mod h1:796sq+UcONnSlzA3RtlBZ+b/hrerkZXiEmO8oMjyRwY=
github.com/grafana/loki/pkg/push v0.0.0-20231124142027-e52380921608 h1:ZYk42718kSXOiIKdjZKljWLgBpzL5z1yutKABksQCMg=
Expand Down
64 changes: 19 additions & 45 deletions lint/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import (
"encoding/json"
"fmt"
"strings"

"github.com/grafana/grafana-foundation-sdk/go/dashboard"
"github.com/grafana/grafana-foundation-sdk/go/stat"
)

type Severity int
Expand Down Expand Up @@ -195,55 +198,26 @@ func (a *Annotation) GetDataSource() (Datasource, error) {
// Panel is a deliberately incomplete representation of the Dashboard -> Panel type in grafana.
// The properties which are extracted from JSON are only those used for linting purposes.
type Panel struct {
Id int `json:"id"`
Title string `json:"title"`
Description string `json:"description,omitempty"`
Targets []Target `json:"targets,omitempty"`
Datasource interface{} `json:"datasource,omitempty"`
Type string `json:"type"`
Panels []Panel `json:"panels,omitempty"`
FieldConfig *FieldConfig `json:"fieldConfig,omitempty"`
}

type FieldConfig struct {
Defaults Defaults `json:"defaults,omitempty"`
Overrides []Override `json:"overrides,omitempty"`
}

type Override struct {
OverrideProperties []OverrideProperty `json:"properties"`
Id int `json:"id"`
Title string `json:"title"`
Description string `json:"description,omitempty"`
Targets []Target `json:"targets,omitempty"`
Datasource interface{} `json:"datasource,omitempty"`
Type string `json:"type"`
Panels []Panel `json:"panels,omitempty"`
FieldConfig *FieldConfig `json:"fieldConfig,omitempty"`
Options json.RawMessage `json:"options,omitempty"`
}

type OverrideProperty struct {
Id string `json:"id"`
Value string `json:"value"`
type statPanel struct {
options stat.Options
Panel
}

func (o *OverrideProperty) UnmarshalJSON(buf []byte) error {
// An override value can be of type string or int
// This function detects type mismatch and uses an int type for Value
var raw struct {
Id string `json:"id"`
Value string `json:"value"`
}

if err := json.Unmarshal(buf, &raw); err != nil {
var raw struct {
Id string `json:"id"`
Value int `json:"value"`
}
if err := json.Unmarshal(buf, &raw); err != nil {
// Override can have varying different types int, string and arrays
// Currently only units are being checked from overrides so returning nil in case of unhandled types
return nil
}
}

return nil
}

type Defaults struct {
Unit string `json:"unit,omitempty"`
type FieldConfig struct {
Defaults dashboard.FieldConfig
//Overrides []Override `json:"overrides,omitempty"`
Overrides []dashboard.DashboardFieldConfigSourceOverrides
}

// GetPanels returns the all panels nested inside the panel (inc the current panel)
Expand Down
69 changes: 63 additions & 6 deletions lint/rule_panel_units.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package lint

import "fmt"
import (
"fmt"

"github.com/grafana/grafana-foundation-sdk/go/dashboard"
"github.com/grafana/grafana-foundation-sdk/go/stat"
)

func NewPanelUnitsRule() *PanelRuleFunc {
validUnits := []string{
Expand Down Expand Up @@ -73,11 +78,37 @@ func NewPanelUnitsRule() *PanelRuleFunc {
configuredUnit := getConfiguredUnit(p)
if configuredUnit != "" {
for _, u := range validUnits {
if u == p.FieldConfig.Defaults.Unit {
if u == *p.FieldConfig.Defaults.Unit {
return r
}
}
}

// ignore if has reduceOptions fields (for stat panels only):
if p.Type == "stat" {
var opts stat.Options
optsByte, err := p.Options.MarshalJSON()
if err != nil {
r.AddError(d, p, err.Error())
}
err = (*stat.Options).UnmarshalJSONStrict(&opts, optsByte)
if err != nil {
r.AddError(d, p, err.Error())
}
statPanel := &statPanel{
Panel: p,
options: opts,
}

if hasReduceOptionsNonNumericFields(*statPanel) {
return r
}
}

//ignore if has value mappings:
if len(getValueMappings(p)) > 0 {
return r
}
r.AddError(d, p, fmt.Sprintf("has no or invalid units defined: '%s'", configuredUnit))
}
return r
Expand All @@ -90,15 +121,41 @@ func getConfiguredUnit(p Panel) string {
// First check if an override with unit exists - if no override then check if standard unit is present and valid
if p.FieldConfig != nil && len(p.FieldConfig.Overrides) > 0 {
for _, p := range p.FieldConfig.Overrides {
for _, o := range p.OverrideProperties {
for _, o := range p.Properties {
if o.Id == "unit" {
configuredUnit = o.Value
configuredUnit = o.Value.(string)
}
}
}
}
if configuredUnit == "" && p.FieldConfig != nil && len(p.FieldConfig.Defaults.Unit) > 0 {
configuredUnit = p.FieldConfig.Defaults.Unit
if configuredUnit == "" && p.FieldConfig != nil && p.FieldConfig.Defaults.Unit != nil {
configuredUnit = *p.FieldConfig.Defaults.Unit
}
return configuredUnit
}

func getValueMappings(p Panel) []dashboard.ValueMapping {
valueMappings := make([]dashboard.ValueMapping, 0)
// First check if an override with unit exists - if no override then check if standard unit is present and valid
if p.FieldConfig != nil && len(p.FieldConfig.Overrides) > 0 {
for _, p := range p.FieldConfig.Overrides {
for _, o := range p.Properties {
if o.Id == "mappings" {
valueMappings = o.Value.([]dashboard.ValueMapping)
}
}
}
}
if len(valueMappings) == 0 && p.FieldConfig != nil && p.FieldConfig.Defaults.Mappings != nil {
valueMappings = *&p.FieldConfig.Defaults.Mappings

Check failure on line 150 in lint/rule_panel_units.go

View workflow job for this annotation

GitHub Actions / lint

SA4001: *&x will be simplified to x. It will not copy x. (staticcheck)
}
return valueMappings
}

func hasReduceOptionsNonNumericFields(p statPanel) bool {

if p.options.ReduceOptions.Fields != nil && *p.options.ReduceOptions.Fields != "" {
return true
}
return false
}
137 changes: 131 additions & 6 deletions lint/rule_panel_units_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,28 @@ package lint

import (
"testing"

"github.com/grafana/grafana-foundation-sdk/go/dashboard"
)

func ptr[T any](t T) *T { return &t }
func TestPanelUnits(t *testing.T) {
linter := NewPanelUnitsRule()

testValueMap := &dashboard.ValueMap{
Type: "value",
Options: map[string]dashboard.ValueMappingResult{
"1": {
Text: ptr("Ok"),
Color: ptr("green"),
},
"2": {
Text: ptr("Down"),
Color: ptr("red"),
},
},
}

for _, tc := range []struct {
name string
result Result
Expand All @@ -23,8 +40,8 @@ func TestPanelUnits(t *testing.T) {
Datasource: "foo",
Title: "bar",
FieldConfig: &FieldConfig{
Defaults: Defaults{
Unit: "MyInvalidUnit",
Defaults: dashboard.FieldConfig{
Unit: ptr("MyInvalidUnit"),
},
},
},
Expand Down Expand Up @@ -62,8 +79,8 @@ func TestPanelUnits(t *testing.T) {
Datasource: "foo",
Title: "bar",
FieldConfig: &FieldConfig{
Defaults: Defaults{
Unit: "short",
Defaults: dashboard.FieldConfig{
Unit: ptr("short"),
},
},
},
Expand All @@ -76,8 +93,116 @@ func TestPanelUnits(t *testing.T) {
Datasource: "foo",
Title: "bar",
FieldConfig: &FieldConfig{
Defaults: Defaults{
Unit: "none",
Defaults: dashboard.FieldConfig{
Unit: ptr("none"),
},
},
},
},
{
name: "has nonnumeric reduceOptions fields",
result: ResultSuccess,
panel: Panel{
Type: "stat",
Datasource: "foo",
Title: "bar",
Options: []byte(`
{
"reduceOptions": {
"values": false,
"calcs": [
"lastNotNull"
],
"fields": "/^version$/"
},
"orientation": "auto",
"textMode": "auto",
"wideLayout": true,
"colorMode": "fixed",
"graphMode": "none",
"justifyMode": "auto",
"showPercentChange": false,
"percentChangeColorMode": "standard"
}
`),
},
},
{
name: "has empty reduceOptions fields(Numeric Fields default value)",
result: Result{
Severity: Error,
Message: "Dashboard 'test', panel 'bar' has no or invalid units defined: ''",
},
panel: Panel{
Type: "stat",
Datasource: "foo",
Title: "bar",
Options: []byte(`
{
"reduceOptions": {
"values": false,
"calcs": [
"lastNotNull"
],
"fields": ""
},
"orientation": "auto",
"textMode": "auto",
"wideLayout": true,
"colorMode": "fixed",
"graphMode": "none",
"justifyMode": "auto",
"showPercentChange": false,
"percentChangeColorMode": "standard"
}
`),
},
},
{
name: "no units but have value mappings",
result: ResultSuccess,
panel: Panel{
Type: "singlestat",
Datasource: "foo",
Title: "bar",
FieldConfig: &FieldConfig{
Defaults: dashboard.FieldConfig{
Mappings: []dashboard.ValueMapOrRangeMapOrRegexMapOrSpecialValueMap{
dashboard.ValueMapOrRangeMapOrRegexMapOrSpecialValueMap{
ValueMap: testValueMap,
},
},
},
},
},
},
{
name: "no units but have value mappings in overrides",
result: ResultSuccess,
panel: Panel{
Type: "singlestat",
Datasource: "foo",
Title: "bar",
FieldConfig: &FieldConfig{
Overrides: []dashboard.DashboardFieldConfigSourceOverrides{
dashboard.DashboardFieldConfigSourceOverrides{
Matcher: dashboard.MatcherConfig{
Id: "byRegexp",
Options: "/.*/",
},
Properties: []dashboard.DynamicConfigValue{
dashboard.DynamicConfigValue{
Id: "mappings",
Value: []dashboard.ValueMapOrRangeMapOrRegexMapOrSpecialValueMap{
dashboard.ValueMapOrRangeMapOrRegexMapOrSpecialValueMap{
ValueMap: testValueMap,
},
},
},
},
},
},
},
},
Expand Down

0 comments on commit 4b54416

Please sign in to comment.