Simplify exporting dashboards for Devs #7730

ruflin · 2018-07-25T09:42:00Z

A new mage command is introduced to simplify exporting dashboards and contributing them to Beats. Currently a contributor is required to know about folder hierarchy under which dashboards are stored and are versioned. To simply this a simple mage command is provided:

MODULE=mysql ID=the-dashboard-id mage ExportDashboard

The Dev must provide the module he wants to export the dashboard for and the dashboard id. The script will take care of extracting it from Kibana, creating directories and files. The current script only supports to export dashboards from localhost:5061 as no params are provided to change the host at the moment. Also this change is only for Metricbeat for now. Filebeat should also be added.

Additional changes:

Create directories
Pass module and dashboard id
Add exit code 1 in case of export failure
Improve error handling

houndci-bot · 2018-07-25T09:42:09Z

metricbeat/magefile.go

@@ -96,6 +98,27 @@ func Fields() error {
 	return mage.GenerateFieldsYAML("module")
 }

+func ExportDashboard() error {


exported function ExportDashboard should have comment or be unexported

houndci-bot · 2018-07-25T09:42:09Z

dev-tools/mage/dashboard.go

+	"github.com/magefile/mage/sh"
+)
+
+func ExportDashboard(id, path string) error {


exported function ExportDashboard should have comment or be unexported

ruflin · 2018-07-25T09:43:37Z

@andrewkroh My first time creating a mage target. Questions:

Should there also be a make command for this or is it ok to only have mage?
Is the way I execute the command the recommended way?

andrewkroh

My approach so far has been to:

Create ExportDashboard(params ExportDashboardArgs) in dev-tools/mage.
Create an ExportDashboardArgs struct with all the available parameters to control the behavior.
Provide a DefaultExportDashboardArgs that returns a ExportDashboardArgs object initialized with the defaults (including reading the env vars, see mage.EnvOr). This it's still possible to override params programmatically if need be.
Update each magefile.go to use the new common method.
func ExportDashboards() error { return mage.ExportDashboards(mage.DefaultExportDashboardsArgs()) }

dev-tools/cmd/dashboards/export_dashboards.go

metricbeat/magefile.go

ruflin · 2018-07-27T10:14:29Z

Unfortunately in this case I think there are no defaults. Do we have a way to make certain ENV variables required? Similar to EnvOr but EnvRequired?

Do you add all magefile commands to the Makefile or would it be ok to have new commands only in mage?

andrewkroh · 2018-07-27T13:30:34Z

Do you add all magefile commands to the Makefile or would it be ok to have new commands only in mage?

No, my goal is just to not break the existing Makefile targets and interface that people depend on. I think we can add mage targets and iterate on them.

Do we have a way to make certain ENV variables required? Similar to EnvOr but EnvRequired?

Not at the moment. And I'm not sure we should. The underlying function that consumes the params would still need to validate them any way. So I'd opt for using EnvOr("MY_VAR", "") then validating the incoming args and returning an informative error message.

ruflin · 2018-08-29T11:01:49Z

@andrewkroh Could you have a look again?

docs/devguide/newdashboards.asciidoc

dev-tools/mage/dashboard.go

metricbeat/magefile.go

andrewkroh

There's a compile issue, but besides that LGTM. This seems worthy of a note in the developer changelog.

dev-tools/mage/dashboard.go

andrewvc · 2018-11-07T20:44:09Z

dev-tools/mage/dashboard.go

+	}
+
+	// TODO: This is currently hardcoded for KB 6, we need to figure out what we do for KB 7
+	file := filepath.Join(beatsDir, BeatName, "module", module, "_meta/kibana/6/dashboard", id+".json")


How does this work with dashboards at locations like https://github.com/elastic/beats/tree/master/heartbeat/monitors/active/http/_meta/kibana/6/dashboard ?

Should those move to this new simpler pattern?

It doesn't at the moment. We should probably make the path a config param which each beat can set, so it also work here (follow up PR).

For the output file you don’t want to use a path that is relative to the ElasticBeatsDir because this won’t work for community beats or x-pack beats. Instead use CWD() or just leave it as a relative path assuming the tool this is being passed to does the write thing and writes the output based on the CWD that it was executed from.

So in short, use file := CWD(“module”, module, “_meta/kibana/6/dashboard”, id+”.json”).

andrewkroh · 2018-11-07T23:23:52Z

x-pack/filebeat/magefile.go would benefit from this new command. Can you please add it there too.

I think this is looking good. We should get it merged soon.

ruflin · 2018-11-09T08:09:43Z

Did rebase and quickly run it locally but exited with 1. Need to investigate what changed.

A new `mage` command is introduced to simplify exporting dashboards and contributing them to Beats. Currently a contributor is required to know about folder hierarchy under which dashboards are stored and are versioned. To simply this a simple `mage` command is provided: ``` MODULE=mysql ID=the-dashboard-id mage ExportDashboard ``` The Dev must provide the module he wants to export the dashboard for and the dashboard id. The script will take care of extracting it from Kibana, creating directories and files. The current script only supports to export dashboards from localhost:5061 as no params are provided to change the host at the moment. Also this change is only for Metricbeat for now. Filebeat should also be added. Additional changes: * Create directories * Pass module and dashboard id * Add exit code 1 in case of export failure * Improve error handling

ruflin · 2018-11-15T01:47:18Z

@andrewkroh Should be ready for an other round of review. Tested now locally and works.

ruflin · 2018-11-15T01:50:27Z

@andrewkroh I tried to add it to x-pack filebeat and it the export worked. But it seems as BeatsDir it takes the one from the OSS Filebeat and puts the data there. I wonder if this is a general issue with the magefile?

I suggest for now we skip this and get this in to then iterate on top of it.

andrewkroh · 2018-11-15T02:43:39Z

@ruflin I see the problem that would prevent you from using it in x-pack and left a comment on how to fix it. Should be a simple fix.

kvch · 2018-11-16T11:09:40Z

If this is merged my PR needs an rebase: #9097
This is a somewhat othogonal change in functionality, but we touch the same files.

ruflin · 2018-11-19T06:58:30Z

@andrewkroh I tried it with your adjustement in the code. It keeps working in OSS but for x-pack it still creates the data in OSS. I would prefer to rather get this PR in as is and get it improved in a follow up PR, especially as this conflicts with the work from @kvch .

andrewkroh

I'm good with merging it in its current state. And then have it fixed for x-pack/*beat in a follow-up PR.

ruflin · 2018-11-20T06:31:55Z

@kvch Merged. You can rebase your PR on top of it now. In case you plan to backport your PR, should we backport this one too to 6.x?

kvch · 2018-11-20T08:33:49Z

I think we can backport it along with the follow-up PR with the fix. But TBH I don't see as much value of this improvement on 6.x than on master. Devs would run it on master or on a branch from master, then backport the dashboards they have exported to 6.x.

ruflin · 2018-11-20T09:03:23Z

@kvch Ok, leave it up to you to decide if you will need this in 6.x or not.

ruflin added in progress Pull request is currently in progress. :Dashboards :Infra labels Jul 25, 2018

ruflin requested a review from andrewkroh July 25, 2018 09:42

houndci-bot reviewed Jul 25, 2018

View reviewed changes

ruflin force-pushed the export-dashboard branch from 3f2525f to 3dfb2af Compare July 25, 2018 09:55

andrewkroh reviewed Jul 26, 2018

View reviewed changes

ruflin force-pushed the export-dashboard branch from 3dfb2af to efeb29f Compare July 27, 2018 10:14

ruflin force-pushed the export-dashboard branch from efeb29f to 9cbdc07 Compare July 27, 2018 14:30

ruflin force-pushed the export-dashboard branch from f81d48c to fb4b20b Compare August 28, 2018 11:20

ruflin added review and removed in progress Pull request is currently in progress. labels Aug 29, 2018

andrewkroh reviewed Aug 29, 2018

View reviewed changes

ruflin force-pushed the export-dashboard branch from dee35d3 to 20dcc51 Compare October 18, 2018 11:49

ruflin added the in progress Pull request is currently in progress. label Oct 18, 2018

andrewkroh reviewed Oct 21, 2018

View reviewed changes

dev-tools/mage/dashboard.go Outdated Show resolved Hide resolved

ruflin mentioned this pull request Nov 7, 2018

Fix export_dashboards command #8954

Merged

andrewvc reviewed Nov 7, 2018

View reviewed changes

ruflin force-pushed the export-dashboard branch from 9ff0847 to 9638612 Compare November 9, 2018 08:07

ruflin force-pushed the export-dashboard branch from 9638612 to 879e807 Compare November 15, 2018 01:35

ruflin force-pushed the export-dashboard branch from a44d482 to f3cb335 Compare November 15, 2018 01:46

ruflin removed the in progress Pull request is currently in progress. label Nov 15, 2018

introduce relative CWD

e9e3e09

andrewkroh approved these changes Nov 19, 2018

View reviewed changes

ruflin merged commit 1cbd201 into elastic:master Nov 20, 2018

ruflin deleted the export-dashboard branch November 20, 2018 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify exporting dashboards for Devs #7730

Simplify exporting dashboards for Devs #7730

ruflin commented Jul 25, 2018 •

edited

Loading

houndci-bot Jul 25, 2018

houndci-bot Jul 25, 2018

ruflin commented Jul 25, 2018

andrewkroh left a comment •

edited

Loading

ruflin commented Jul 27, 2018

andrewkroh commented Jul 27, 2018

ruflin commented Aug 29, 2018

andrewkroh left a comment

andrewvc Nov 7, 2018

ruflin Nov 9, 2018

andrewkroh Nov 15, 2018

andrewkroh commented Nov 7, 2018

ruflin commented Nov 9, 2018

ruflin commented Nov 15, 2018

ruflin commented Nov 15, 2018

andrewkroh commented Nov 15, 2018

kvch commented Nov 16, 2018

ruflin commented Nov 19, 2018

andrewkroh left a comment

ruflin commented Nov 20, 2018

kvch commented Nov 20, 2018

ruflin commented Nov 20, 2018

Simplify exporting dashboards for Devs #7730

Simplify exporting dashboards for Devs #7730

Conversation

ruflin commented Jul 25, 2018 • edited Loading

houndci-bot Jul 25, 2018

Choose a reason for hiding this comment

houndci-bot Jul 25, 2018

Choose a reason for hiding this comment

ruflin commented Jul 25, 2018

andrewkroh left a comment • edited Loading

Choose a reason for hiding this comment

ruflin commented Jul 27, 2018

andrewkroh commented Jul 27, 2018

ruflin commented Aug 29, 2018

andrewkroh left a comment

Choose a reason for hiding this comment

andrewvc Nov 7, 2018

Choose a reason for hiding this comment

ruflin Nov 9, 2018

Choose a reason for hiding this comment

andrewkroh Nov 15, 2018

Choose a reason for hiding this comment

andrewkroh commented Nov 7, 2018

ruflin commented Nov 9, 2018

ruflin commented Nov 15, 2018

ruflin commented Nov 15, 2018

andrewkroh commented Nov 15, 2018

kvch commented Nov 16, 2018

ruflin commented Nov 19, 2018

andrewkroh left a comment

Choose a reason for hiding this comment

ruflin commented Nov 20, 2018

kvch commented Nov 20, 2018

ruflin commented Nov 20, 2018

ruflin commented Jul 25, 2018 •

edited

Loading

andrewkroh left a comment •

edited

Loading