Add warning/error for dashboards including visualizations by reference #389

mrodm · 2022-08-12T11:35:49Z

What does this PR do?

Add a new validator to check about dashboards using visualizations by reference instead of value. Example output from tests:

=== RUN   TestValidateFile/visualizations_by_reference
2022/08/17 13:01:10 Warning: references found in dashboard kibana/dashboard/visualizations_by_reference-82273ffe-6acc-4f2f-bbee-c1004abba63d.json: visualizations_by_reference-5e1a01ff-6f9a-41c1-b7ad-326472db42b6 (visualization), visualizations_by_reference-8287a5d5-1576-4f3a-83c4-444e9058439b (lens)

These references are obtained from the dashboard json file:

{
  ...
  "id": "dashboard-id",
  "references": [
    {
      "id": "visualizations_by_reference-5e1a01ff-6f9a-41c1-b7ad-326472db42b6",
      "name": "panel_0",
      "type": "visualization"
    },
    {
      "id": "visualizations_by_reference-8287a5d5-1576-4f3a-83c4-444e9058439b",
      "name": "panel_1",
      "type": "visualization"
    }
  ],
  "type": "dashboard"
}

Why is it important?

This new check wants to highlight (using warning messages) those dashboards using still visualizations by reference.

As mentioned in #316:

The goal is to move to by value to have dashboards and visualizations directly bundled together as a unit which speeds up the installation of it in Fleet and also makes sure the visualizations do not all show up in the search (visualization library), only the dashboard.

Checklist

I have added test packages to test/packages that prove my change is effective.
I have added an entry in versions/N/changelog.yml.

Related issues

Closes Warning when dashboard with visualisations by reference instead of value exist #316

elasticmachine · 2022-08-12T11:38:58Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-08-18T11:26:10.190+0000
Duration: 7 min 59 sec

Test stats 🧪

Test	Results
Failed	0
Passed	391
Skipped	2
Total	393

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

elasticmachine · 2022-08-12T13:57:47Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`7/7`)	💚
Files	70.833% (`17/24`)	👍 2.652
Classes	78.788% (`26/33`)	👍 1.369
Methods	58.416% (`59/101`)	👍 0.521
Lines	43.876% (`523/1192`)	👎 -0.186
Conditionals	100.0% (`0/0`)	💚

mrodm · 2022-08-12T15:53:18Z

/test

mrodm · 2022-08-16T07:45:48Z

/test

mrodm · 2022-08-16T16:25:59Z

code/go/internal/validator/semantic/validate_dashboards_used_by_value.go

+	for _, objectFile := range objectFiles {
+		filePath := objectFile.Path()
+
+		objectReferences, err := objectFile.Values(`$.references[?(@.type=="visualization")]`)


is this the right way to check for visualizations included by reference in a dashboard?

I guess that the best will be to have some test cases 🙂

I would want to confirm that this reference key in the dashboard json files is the one defining that a visualization is set as reference.

I've just tested that adding some visualizations from the library in the EPR integration, and it resulted in these changes:

--- packages/elastic_package_registry/kibana/dashboard/elastic_package_registry-313c2700-099b-11ed-91b6-3b1f9c2b2771.json +++ packages/elastic_package_registry/kibana/dashboard/elastic_package_registry-313c2700-099b-11ed-91b6-3b1f9c2b2771.json @@ -2215,6 +2215,38 @@ "title": "EPR Indexer - Get duration seconds per indexer - Percentile 95", "type": "lens", "version": "8.0.0" + }, + { + "embeddableConfig": { + "enhancements": {} + }, + "gridData": { + "h": 15, + "i": "6beb6552-317f-442e-a6d5-397dc849aa98", + "w": 24, + "x": 0, + "y": 82 + }, + "panelIndex": "6beb6552-317f-442e-a6d5-397dc849aa98", + "panelRefName": "panel_6beb6552-317f-442e-a6d5-397dc849aa98", + "type": "lens", + "version": "8.0.0" + }, + { + "embeddableConfig": { + "enhancements": {} + }, + "gridData": { + "h": 15, + "i": "a3ae1ef0-3e98-4d2c-9b19-8b2ddc54ef4d", + "w": 24, + "x": 24, + "y": 82 + }, + "panelIndex": "a3ae1ef0-3e98-4d2c-9b19-8b2ddc54ef4d", + "panelRefName": "panel_a3ae1ef0-3e98-4d2c-9b19-8b2ddc54ef4d", + "type": "visualization", + "version": "8.0.0" } ], "refreshInterval": { @@ -2417,6 +2449,16 @@ "id": "metrics-*", "name": "d237dd46-1074-47ee-8186-3957b956acd9:indexpattern-datasource-layer-cfdc23c6-0606-45ea-b1d2-782ba8103f82", "type": "index-pattern" + }, + { + "id": "elastic_package_registry-b82baae0-1e06-11ed-92f0-1397dcee01a8", + "name": "6beb6552-317f-442e-a6d5-397dc849aa98:panel_6beb6552-317f-442e-a6d5-397dc849aa98", + "type": "lens" + }, + { + "id": "elastic_package_registry-fd226440-1e06-11ed-92f0-1397dcee01a8", + "name": "a3ae1ef0-3e98-4d2c-9b19-8b2ddc54ef4d:panel_a3ae1ef0-3e98-4d2c-9b19-8b2ddc54ef4d", + "type": "visualization" } ], "type": "dashboard"

From this test, I've just realised that it should be checked whether or not there are references from both Lens and Visualization. Should it be checked for other types too ?

I will try to add some test cases also for this part too (getting the reference elements from the json)

mrodm · 2022-08-16T16:41:23Z

code/go/pkg/validator/validator_test.go

@@ -90,6 +90,7 @@ func TestValidateFile(t *testing.T) {
 				"field services.docker-custom-agent: Must not validate the schema (not)",
 			},
 		},
+		"visualizations_by_reference": {},


Adding this new element here, the package will be tested, but no errors can be checked. Should it be moved to another test ?

Good point 🤔 Maybe we need to add some way to inject loggers so we can capture what is being logged. But we can also ignore this till we have #342.

Maybe we can have some undocumented environment variable like PACKAGE_SPEC_WARNINGS_AS_ERRORS that produces errors on warnings. We could enable it in tests.

Added that environment variable with some helpers to be able to do tests on warnings
mrodm@06f27a9

jsoriano · 2022-08-16T17:22:05Z

code/go/internal/validator/semantic/validate_dashboards_used_by_value.go

+			continue
+		}
+
+		anyReference(objectReferences, fsys.Path(filePath))


Do you want to check the returning values of this call? Or this is only to log till we have a better way of reporting warnings?

Consider in any case explictly ignoring the returned values.

Suggested change

anyReference(objectReferences, fsys.Path(filePath))

_, _ := anyReference(objectReferences, fsys.Path(filePath))

Or move error reporting here, something like this:

Suggested change

anyReference(objectReferences, fsys.Path(filePath))

ids, err := anyReference(objectReferences, fsys.Path(filePath))

if err != nil {

return err

}

if len(ids) > 0 {

log.Printf("Warning: visualization by reference found in %s: %s", filePath, strings.Join(ids, ", "))

}

Updated this function to return references and errors (if any). With this change, ValidateVisualizationsUsedByValue function is the one in charge of showing the warnings.

jsoriano · 2022-08-16T17:24:07Z

code/go/internal/validator/semantic/validate_dashboards_used_by_value.go

+	for _, objectFile := range objectFiles {
+		filePath := objectFile.Path()
+
+		objectReferences, err := objectFile.Values(`$.references[?(@.type=="visualization")]`)


I guess that the best will be to have some test cases 🙂

jsoriano · 2022-08-16T17:29:04Z

code/go/internal/validator/semantic/validate_dashboards_used_by_value.go

+func anyReference(val interface{}, path string) (bool, error) {
+	references, err := toReferenceSlice(val)
+	if err != nil {
+		return false, fmt.Errorf("unable to convert references in file [%s]", path)


This looks like an error that we don't want to ignore when calling anyReference.

jsoriano · 2022-08-16T17:34:48Z

code/go/internal/validator/semantic/validate_dashboards_used_by_value.go

+
+func toReferenceSlice(val interface{}) ([]reference, error) {
+	var refs []reference
+	jsonbody, err := json.Marshal(val)


Nit. Probably we can expect a type like map[string]interface{} for these values, maybe you can convert from there instead of marshalling and unmarshalling.

This library may also help to convert from a generic map to a struct: https://pkg.go.dev/github.com/mitchellh/mapstructure

I've changed the code to use casting/expected types. For the moment, as it would be just one usage, I've tried to not use that mapstructure library.

jsoriano · 2022-08-16T17:35:42Z

code/go/internal/validator/semantic/validate_dashboards_used_by_value.go

+func anyReference(val interface{}, path string) (bool, error) {
+	references, err := toReferenceSlice(val)
+	if err != nil {
+		return false, fmt.Errorf("unable to convert references in file [%s]", path)


Wrap the error here.

Suggested change

return false, fmt.Errorf("unable to convert references in file [%s]", path)

return false, fmt.Errorf("unable to convert references in file [%s]: %w", path, err)

jsoriano · 2022-08-16T17:36:14Z

code/go/internal/validator/semantic/validate_dashboards_used_by_value.go

+
+	err = json.Unmarshal(jsonbody, &refs)
+	if err != nil {
+		log.Printf("error unmarshaling references: %s", err)


Why logging these errors? I think it would be better to return the errors and handle (or ignore) in the callers.

These conversion errors are now returned and the callers can handle them properly.

jsoriano · 2022-08-16T17:40:18Z

code/go/pkg/validator/validator_test.go

@@ -90,6 +90,7 @@ func TestValidateFile(t *testing.T) {
 				"field services.docker-custom-agent: Must not validate the schema (not)",
 			},
 		},
+		"visualizations_by_reference": {},


Good point 🤔 Maybe we need to add some way to inject loggers so we can capture what is being logged. But we can also ignore this till we have #342.

Update also messages shown to indicate both the id and type of the reference found.

mrodm · 2022-08-17T10:43:50Z

code/go/internal/validator/semantic/validate_visualizations_used_by_value.go

+
+		objectReferences, err := objectFile.Values(`$.references`)
+		if err != nil {
+			// no references key in dashboard json


There are test packages that do not have "references" key. To avoid errors in case of this key does not exist, this error is ignored.

mrodm · 2022-08-17T11:02:43Z

code/go/internal/validator/semantic/validate_visualizations_used_by_value.go

+			for _, ref := range references[1:] {
+				s = fmt.Sprintf("%s, %s (%s)", s, ref.ID, ref.Type)
+			}
+			log.Printf("Warning: references found in dashboard %s: %s", filePath, s)


Example of output here:

2022/08/17 13:01:10 Warning: references found in dashboard kibana/dashboard/visualizations_by_reference-82273ffe-6acc-4f2f-bbee-c1004abba63d.json: visualizations_by_reference-5e1a01ff-6f9a-41c1-b7ad-326472db42b6 (visualization), visualizations_by_reference-8287a5d5-1576-4f3a-83c4-444e9058439b (lens)

jsoriano · 2022-08-18T08:30:03Z

code/go/internal/validator/common/helpers.go

+	"strconv"
+)
+
+// EnvVarWarningsAsErrors is the environment variable name used to use warnings as errors


Please add a comment here mentioning that this mechanism will be removed if/when structured errors are supported.

jsoriano · 2022-08-18T08:35:09Z

code/go/internal/validator/spec_test.go

 	"testing"

+	"github.com/elastic/package-spec/code/go/internal/fspath"
+


Nit. Move package spec imports to the bottom of the import block.

I thought goimports would reorder it and I didn't check that. Updated.
Thanks!

jsoriano · 2022-08-18T08:36:08Z

code/go/pkg/validator/validator_test.go

@@ -28,9 +29,6 @@ func TestValidateFile(t *testing.T) {
 		"deploy_terraform":    {},
 		"time_series":         {},
 		"missing_data_stream": {},
-		"custom_logs":         {},
-		"httpjson_input":      {},
-		"sql_input":           {},


Could we keep these packages here if we are not enabling errors as warnings here?

If I keep those packages here, they are going to be checked twice.
In the new test suite where warnings are considered as errors too, if there is any error that is raised (and not expected), then the test should fail too as they do no expect any error ({}).

For instance, I added a new field foo in the manifest in one of them:

=== RUN TestValidateWarnings/custom_logs validator_test.go:334: Error Trace: validator_test.go:334 Error: "found 2 validation errors: 1. Warning: package with non-stable semantic version and active beta features (enabled in [../../../../test/packages/custom_logs]) can't be released as stable version. 2. file "../../../../test/packages/custom_logs/manifest.yml" is invalid: field (root): Additional property foo is not allowed " should have 1 item(s), but has 2 Test: TestValidateWarnings/custom_logs

I could keep them in this test suite, in case the new test suite gets removed after structured errors are implemented. WDYT ? @jsoriano

Ok, you are right, it is probably fine as it is. If we add structured errors we probably need to revisit these tests in any case.

jsoriano · 2022-08-18T08:37:28Z

code/go/pkg/validator/validator_test.go

+	if err := common.DisableWarningsAsErrors(); err != nil {
+		require.NoError(t, err)
+	}


Nit. Run it in a defer after enabling this?

Changed using defer

mrodm · 2022-08-18T09:49:12Z

/test

Add warning/error for dashboards with visualization by value

7ffa200

mrodm added the Team:Ecosystem Label for the Packages Ecosystem team label Aug 12, 2022

mrodm self-assigned this Aug 12, 2022

mrodm added 4 commits August 12, 2022 15:49

fix lint

d4665f0

add changelog

04b0262

Rename validate method

cdf5076

rename file

5710b44

Add tests

42917c0

Use json path to get visualization references

84211e0

mrodm commented Aug 16, 2022

View reviewed changes

mrodm changed the title ~~Add warning/error for dashboards including visualizations by reference - WIP~~ Add warning/error for dashboards including visualizations by reference Aug 16, 2022

mrodm marked this pull request as ready for review August 16, 2022 16:42

mrodm requested a review from a team as a code owner August 16, 2022 16:42

jsoriano reviewed Aug 16, 2022

View reviewed changes

mrodm added 3 commits August 17, 2022 11:33

Test lens and visualization

b51efdd

Update toReferenceSlice

67886df

Update also messages shown to indicate both the id and type of the reference found.

Rename files

bf27852

mrodm commented Aug 17, 2022

View reviewed changes

fix arguments

008ec0d

mrodm commented Aug 17, 2022

View reviewed changes

Add a new env.var to enable transform warnings into errors for testing

06f27a9

mrodm requested a review from jsoriano August 17, 2022 17:18

mrodm added 2 commits August 18, 2022 10:11

Merge remote branch upstream/main into warning_dashbaords_reference

82338f0

Add comments and license header

2f389f9

jsoriano reviewed Aug 18, 2022

View reviewed changes

mrodm added 3 commits August 18, 2022 12:08

Add comment about removal depending on structured errors

8654306

reorder imports

e8a9c56

Use defer to disable warnings as errors in test

d78a905

jsoriano previously approved these changes Aug 18, 2022

View reviewed changes

Use path instead of filepath for test

9886386

mrodm dismissed jsoriano’s stale review via 9886386 August 18, 2022 11:25

mrodm requested a review from jsoriano August 18, 2022 14:10

jsoriano approved these changes Aug 18, 2022

View reviewed changes

mrodm merged commit 039ad7e into elastic:main Aug 18, 2022

mrodm deleted the warning_dashboards_reference branch August 18, 2022 14:51

This was referenced Sep 13, 2022

Warning when dashboard with visualisations by reference instead of value exist #316

Closed

Add map as another reference type to check to show warnings #414

Merged

jsoriano mentioned this pull request Oct 11, 2022

[AWS] Add Kinesis Metrics datastream elastic/integrations#3829

Merged

4 tasks

shmsr mentioned this pull request Oct 11, 2023

[apache_tomcat] - Migrate to package-spec v3 elastic/integrations#8159

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning/error for dashboards including visualizations by reference #389

Add warning/error for dashboards including visualizations by reference #389

mrodm commented Aug 12, 2022 •

edited

Loading

elasticmachine commented Aug 12, 2022 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented Aug 12, 2022 •

edited

Loading

mrodm commented Aug 12, 2022

mrodm commented Aug 16, 2022

mrodm Aug 16, 2022

jsoriano Aug 16, 2022

mrodm Aug 17, 2022 •

edited

Loading

mrodm Aug 16, 2022

jsoriano Aug 16, 2022

jsoriano Aug 17, 2022 •

edited

Loading

mrodm Aug 17, 2022

jsoriano Aug 16, 2022

mrodm Aug 17, 2022

jsoriano Aug 16, 2022

jsoriano Aug 16, 2022

jsoriano Aug 16, 2022

mrodm Aug 17, 2022

jsoriano Aug 16, 2022

jsoriano Aug 16, 2022

mrodm Aug 17, 2022

jsoriano Aug 16, 2022

mrodm Aug 17, 2022

mrodm Aug 17, 2022

jsoriano Aug 18, 2022

jsoriano Aug 18, 2022

mrodm Aug 18, 2022

jsoriano Aug 18, 2022

mrodm Aug 18, 2022

jsoriano Aug 18, 2022

jsoriano Aug 18, 2022

mrodm Aug 18, 2022

mrodm commented Aug 18, 2022

	anyReference(objectReferences, fsys.Path(filePath))
	_, _ := anyReference(objectReferences, fsys.Path(filePath))

-		anyReference(objectReferences, fsys.Path(filePath))
+	ids, err := anyReference(objectReferences, fsys.Path(filePath))
+	if err != nil {
+		return err
+	}
+	if len(ids) > 0 {
+		log.Printf("Warning: visualization by reference found in %s: %s", filePath, strings.Join(ids, ", "))
+	}

	return false, fmt.Errorf("unable to convert references in file [%s]", path)
	return false, fmt.Errorf("unable to convert references in file [%s]: %w", path, err)

		"testing"

		"github.com/elastic/package-spec/code/go/internal/fspath"

Add warning/error for dashboards including visualizations by reference #389

Add warning/error for dashboards including visualizations by reference #389

Conversation

mrodm commented Aug 12, 2022 • edited Loading

What does this PR do?

Why is it important?

Checklist

Related issues

elasticmachine commented Aug 12, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

elasticmachine commented Aug 12, 2022 • edited Loading

🌐 Coverage report

mrodm commented Aug 12, 2022

mrodm commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrodm Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrodm commented Aug 18, 2022

mrodm commented Aug 12, 2022 •

edited

Loading

elasticmachine commented Aug 12, 2022 •

edited

Loading

elasticmachine commented Aug 12, 2022 •

edited

Loading

mrodm Aug 17, 2022 •

edited

Loading

jsoriano Aug 17, 2022 •

edited

Loading