Commit cc97f9ee authored by Mikolaj Wawrzyniak's avatar Mikolaj Wawrzyniak

Improve dashboard validation error messages

Improve metrics dashboard validation error messages, to be easier
to understand from user perspective.
parent 82678ecc
......@@ -34,7 +34,7 @@ module Gitlab
end
def schemer
@schemer ||= JSONSchemer.schema(Pathname.new(schema_path), formats: custom_formats.format_handlers)
@schemer ||= ::JSONSchemer.schema(Pathname.new(schema_path), formats: custom_formats.format_handlers)
end
def validate_against_schema
......
......@@ -9,19 +9,42 @@ module Gitlab
class SchemaValidationError < InvalidDashboardError
def initialize(error = {})
if error.is_a?(Hash) && error.present?
data = error["data"]
data_pointer = error["data_pointer"]
schema = error["schema"]
schema_pointer = error["schema_pointer"]
super(error_message(error))
end
msg = _("'%{data}' is invalid at '%{data_pointer}'. Should be '%{schema}' due to schema definition at '%{schema_pointer}'") %
{ data: data, data_pointer: data_pointer, schema: schema, schema_pointer: schema_pointer }
private
def error_message(error)
if error.is_a?(Hash) && error.present?
pretty(error)
else
msg = "Dashboard failed schema validation"
"Dashboard failed schema validation"
end
end
# based on https://github.com/davishmcclurg/json_schemer/blob/master/lib/json_schemer/errors.rb
# with addition ability to translate error messages
def pretty(error)
data, data_pointer, type, schema = error.values_at('data', 'data_pointer', 'type', 'schema')
location = data_pointer.empty? ? 'root' : data_pointer
super(msg)
case type
when 'required'
keys = error.fetch('details').fetch('missing_keys').join(', ')
_("%{location} is missing required keys: %{keys}") % { location: location, keys: keys }
when 'null', 'string', 'boolean', 'integer', 'number', 'array', 'object'
_("'%{data}' at %{location} is not of type: %{type}") % { data: data, location: location, type: type }
when 'pattern'
_("'%{data}' at %{location} does not match pattern: %{pattern}") % { data: data, location: location, pattern: schema.fetch('pattern') }
when 'format'
_("'%{data}' at %{location} does not match format: %{format}") % { data: data, location: location, format: schema.fetch('format') }
when 'const'
_("'%{data}' at %{location} is not: %{const}") % { data: data, location: location, const: schema.fetch('const').inspect }
when 'enum'
_("'%{data}' at %{location} is not one of: %{enum}") % { data: data, location: location, enum: schema.fetch('enum') }
else
_("'%{data}' at %{location} is invalid: error_type=%{type}") % { data: data, location: location, type: type }
end
end
end
......
......@@ -524,6 +524,9 @@ msgstr ""
msgid "%{loadingIcon} Started"
msgstr ""
msgid "%{location} is missing required keys: %{keys}"
msgstr ""
msgid "%{lock_path} is locked by GitLab User %{lock_user_id}"
msgstr ""
......@@ -803,7 +806,22 @@ msgstr ""
msgid "&lt;project name&gt;"
msgstr ""
msgid "'%{data}' is invalid at '%{data_pointer}'. Should be '%{schema}' due to schema definition at '%{schema_pointer}'"
msgid "'%{data}' at %{location} does not match format: %{format}"
msgstr ""
msgid "'%{data}' at %{location} does not match pattern: %{pattern}"
msgstr ""
msgid "'%{data}' at %{location} is invalid: error_type=%{type}"
msgstr ""
msgid "'%{data}' at %{location} is not of type: %{type}"
msgstr ""
msgid "'%{data}' at %{location} is not one of: %{enum}"
msgstr ""
msgid "'%{data}' at %{location} is not: %{const}"
msgstr ""
msgid "'%{level}' is not a valid visibility level"
......
dashboard: 'Test Dashboard'
panel_groups:
- panels:
- title: "Super Chart A1"
type: "area-chart"
y_label: "y_label"
weight: 1
max_value: 1
metrics:
- id: metric_a1
query_range: |+
avg(
sum(
container_memory_usage_bytes{
container_name!="POD",
pod_name=~"^{{ci_environment_slug}}-(.*)",
namespace="{{kube_namespace}}"
}
) by (job)
) without (job)
/1024/1024/1024
unit: unit
label: Legend Label
- title: "Super Chart A2"
type: "area-chart"
y_label: "y_label"
weight: 2
metrics:
- id: metric_a2
query_range: 'query'
label: Legend Label
unit: unit
- group: Group B
---
- dashboard: 'Test Dashboard'
panel_groups:
- group: Group A
panels:
- title: "Super Chart A2"
type: "area-chart"
y_label: "y_label"
weight: 2
metrics:
- id: metric_a2
query_range: 'query'
label: Legend Label
unit: unit
- dashboard: 'second entry'
dashboard: 'Test Dashboard'
priority: 1
links:
- title: Link 1
url: https://gitlab.com
- title: Link 2
url: https://docs.gitlab.com
templating:
variables:
text_variable_full_syntax:
label: 'Variable 1'
type: text
options:
default_value: 'default'
text_variable_simple_syntax: 'default value'
custom_variable_simple_syntax: ['value1', 'value2', 'value3']
custom_variable_full_syntax:
label: 'Variable 2'
type: custom
options:
values:
- value: 'value option 1'
text: 'Option 1'
- value: 'value_option_2'
text: 'Option 2'
default: true
metric_label_values_variable:
label: 'Variable 3'
type: metric_label_values
options:
series_selector: 'backend:haproxy_backend_availability:ratio{env="{{env}}"}'
label: 'backend'
dashboard: 'Test Dashboard'
panel_groups:
- group: Group A
priority: 1
panels:
- title: "Super Chart A1"
type: "area-chart"
y_label: "y_label"
weight: 1
max_value: 1
- title: "Super Chart A2"
type: "area-chart"
y_label: "y_label"
weight: 2
metrics:
dashboard: 'Test Dashboard'
priority: 1
links:
- title: Link 1
url: https://gitlab.com
- title: Link 2
url: https://docs.gitlab.com
templating:
variables:
text_variable_full_syntax:
label: 'Variable 1'
type: text
options:
default_value: 'default'
text_variable_simple_syntax: 'default value'
custom_variable_simple_syntax: ['value1', 'value2', 'value3']
custom_variable_full_syntax:
label: 'Variable 2'
type: custom
options:
values:
- value: 'value option 1'
text: 'Option 1'
- value: 'value_option_2'
text: 'Option 2'
default: true
metric_label_values_variable:
label: 'Variable 3'
type: metric_label_values
options:
series_selector: 'backend:haproxy_backend_availability:ratio{env="{{env}}"}'
label: 'backend'
panel_groups: this should be an array
......@@ -4,28 +4,130 @@ require 'spec_helper'
RSpec.describe Gitlab::Metrics::Dashboard::Validator::Errors do
describe Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError do
context 'valid error hash from jsonschemer' do
context 'empty error hash' do
let(:error_hash) { {} }
it 'uses default error message' do
expect(described_class.new(error_hash).message).to eq('Dashboard failed schema validation')
end
end
context 'formatted message' do
subject { described_class.new(error_hash).message }
let(:error_hash) do
{
'data' => 'data',
'data_pointer' => 'data_pointer',
'data' => 'property_name',
'data_pointer' => pointer,
'type' => type,
'schema' => 'schema',
'schema_pointer' => 'schema_pointer'
'details' => details
}
end
it 'formats message' do
expect(described_class.new(error_hash).message).to eq(
"'data' is invalid at 'data_pointer'. Should be 'schema' due to schema definition at 'schema_pointer'"
)
context 'for root object' do
let(:pointer) { '' }
context 'when required keys are missing' do
let(:type) { 'required' }
let(:details) { { 'missing_keys' => ['one'] } }
it { is_expected.to eq 'root is missing required keys: one' }
end
end
context 'empty error hash' do
let(:error_hash) { {} }
context 'for nested object' do
let(:pointer) { '/nested_objects/0' }
it 'uses default error message' do
expect(described_class.new(error_hash).message).to eq('Dashboard failed schema validation')
context 'when required keys are missing' do
let(:type) { 'required' }
let(:details) { { 'missing_keys' => ['two'] } }
it { is_expected.to eq '/nested_objects/0 is missing required keys: two' }
end
context 'when there is type mismatch' do
%w(null string boolean integer number array object).each do |expected_type|
context "on type: #{expected_type}" do
let(:type) { expected_type }
let(:details) { nil }
subject { described_class.new(error_hash).message }
it { is_expected.to eq "'property_name' at /nested_objects/0 is not of type: #{expected_type}" }
end
end
end
context 'when data does not match pattern' do
let(:type) { 'pattern' }
let(:error_hash) do
{
'data' => 'property_name',
'data_pointer' => pointer,
'type' => type,
'schema' => { 'pattern' => 'aa.*' }
}
end
it { is_expected.to eq "'property_name' at /nested_objects/0 does not match pattern: aa.*" }
end
context 'when data does not match format' do
let(:type) { 'format' }
let(:error_hash) do
{
'data' => 'property_name',
'data_pointer' => pointer,
'type' => type,
'schema' => { 'format' => 'date-time' }
}
end
it { is_expected.to eq "'property_name' at /nested_objects/0 does not match format: date-time" }
end
context 'when data is not const' do
let(:type) { 'const' }
let(:error_hash) do
{
'data' => 'property_name',
'data_pointer' => pointer,
'type' => type,
'schema' => { 'const' => 'one' }
}
end
it { is_expected.to eq "'property_name' at /nested_objects/0 is not: \"one\"" }
end
context 'when data is not included in enum' do
let(:type) { 'enum' }
let(:error_hash) do
{
'data' => 'property_name',
'data_pointer' => pointer,
'type' => type,
'schema' => { 'enum' => %w(one two) }
}
end
it { is_expected.to eq "'property_name' at /nested_objects/0 is not one of: [\"one\", \"two\"]" }
end
context 'when data is not included in enum' do
let(:type) { 'unknown' }
let(:error_hash) do
{
'data' => 'property_name',
'data_pointer' => pointer,
'type' => type,
'schema' => 'schema'
}
end
it { is_expected.to eq "'property_name' at /nested_objects/0 is invalid: error_type=unknown" }
end
end
end
end
......
......@@ -34,6 +34,15 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator do
end
describe '#validate!' do
shared_examples 'validation failed' do |errors_message|
it 'raises error with corresponding messages', :aggregate_failures do
expect { subject }.to raise_error do |error|
expect(error).to be_kind_of(Gitlab::Metrics::Dashboard::Validator::Errors::InvalidDashboardError)
expect(error.message).to eq(errors_message)
end
end
end
context 'valid dashboard' do
it 'returns true' do
expect(described_class.validate!(valid_dashboard)).to be true
......@@ -41,22 +50,37 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator do
end
context 'invalid dashboard' do
subject { described_class.validate!(invalid_dashboard) }
context 'invalid schema' do
it 'raises error' do
expect { described_class.validate!(invalid_dashboard) }
.to raise_error(Gitlab::Metrics::Dashboard::Validator::Errors::InvalidDashboardError,
"'this_should_be_a_int' is invalid at '/panel_groups/0/panels/0/weight'."\
" Should be '{\"type\"=>\"number\"}' due to schema definition at '/properties/weight'")
context 'wrong property type' do
it_behaves_like 'validation failed', "'this_should_be_a_int' at /panel_groups/0/panels/0/weight is not of type: number"
end
context 'panel groups missing' do
let_it_be(:invalid_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/dashboard_missing_panel_groups.yml')) }
it_behaves_like 'validation failed', 'root is missing required keys: panel_groups'
end
context 'groups are missing panels and group keys' do
let_it_be(:invalid_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/dashboard_groups_missing_panels_and_group.yml')) }
it_behaves_like 'validation failed', '/panel_groups/0 is missing required keys: group'
end
context 'panel is missing metrics key' do
let_it_be(:invalid_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/dashboard_panel_is_missing_metrics.yml')) }
it_behaves_like 'validation failed', '/panel_groups/0/panels/0 is missing required keys: metrics'
end
end
context 'duplicate metric ids' do
context 'with no project given' do
it 'checks against given dashboard and returns false' do
expect { described_class.validate!(duplicate_id_dashboard) }
.to raise_error(Gitlab::Metrics::Dashboard::Validator::Errors::InvalidDashboardError,
"metric_id must be unique across a project")
end
subject { described_class.validate!(duplicate_id_dashboard) }
it_behaves_like 'validation failed', 'metric_id must be unique across a project'
end
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment