Skip to content

Unify dashboard exporter tools#9097

Merged
kvch merged 15 commits intoelastic:masterfrom
kvch:refactoring-libbeat-unify-export-dashboards
Nov 27, 2018
Merged

Unify dashboard exporter tools#9097
kvch merged 15 commits intoelastic:masterfrom
kvch:refactoring-libbeat-unify-export-dashboards

Conversation

@kvch
Copy link
Copy Markdown
Contributor

@kvch kvch commented Nov 15, 2018

The existing export_dashboards.go and the command export dashboards are unified, using the same code and slightly different logic. Configuration of export_dashboards is done using the following command line options: -kibana, -space, -output, -quiet and -index-pattern. The last flag is an odd one out, because it is part of the script, but its value is never used. So far no one complained, so I did not port it. The default value of that flag is the same in case of both methods.

Configuration the Kibana client of export dashboard is read from the config of the Beat. Thus, Kibana-related flags are not part of its CLI.

By default export dashboard does not decode the exported dashboard. If flag -decode passed to the command, the dashboard is decoded.

Equivalent commands

export dashboards from a dashboards.yml

$ ./filebeat export dashboard -yml path/to/dashboards.yml -decode
$ go run dev-tools/cmd/dashboards/export_dashboards.go -yml path/to/dashboards.yml

export a dashboard with an id and print to stdout

$ ./filebeat export dashboard -id {uuid} -decode
$ go run dev-tools/cmd/dashboards/export_dashboards.go -dashboard {uuid}

The interface of export dashboard is different than in the previous PR, because I wanted to follow existing methods and documentation.

TODO

  • add automated tests
  • add new export dashboard option to dev docs

@kvch kvch added in progress Pull request is currently in progress. refactoring libbeat labels Nov 15, 2018
@kvch kvch requested review from ph and urso November 15, 2018 13:51
@kvch kvch added review needs_backport PR is waiting to be backported to other branches. labels Nov 15, 2018
@kvch kvch removed the in progress Pull request is currently in progress. label Nov 16, 2018
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this PR be backported? If not, we could remove everything related to 5.x

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is planned to as the label suggests.

Copy link
Copy Markdown
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good work @kvch and thanks for adding integration tests. I have added a few minors things.

I see we test for happy path in the integration tests could we add tests for non-happy-path like when dashboard don't exist?

@kvch
Copy link
Copy Markdown
Contributor Author

kvch commented Nov 21, 2018

I added more E2E tests which cover requesting unknown dashboards and invalid files.

Copy link
Copy Markdown
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a note about my mistake in parsing URL :(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a mistake, url.Parse or url.ParseRequestURL is well really bad see for yourself:

https://play.golang.org/p/9NAU0-sasfT

I think we have to come up with something better, maybe we already have in other outputs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In elasticsearch output url.Parse is used. Looking at the codebase, everytime we connect to something url.Parse is used. I think it is good enough.

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Nov 23, 2018

If I remember correctly the beat export dashboard command did export the dashboards without decoding to make sure they can be directly imported again. The dev export command also did the decoding to make it better for versioning. I skimmed through the code and it seems now in both cases the decoding happens? Didn't test locally.

@kvch
Copy link
Copy Markdown
Contributor Author

kvch commented Nov 23, 2018

@ruflin That's exactly how it worked. I added the option -decode to export dashboard, so users can decide if they want a decoded version or not. I left the functionality of export_dashboard.go as is.

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Nov 23, 2018

@kvch This is great. Didn't get to review the code itself yet but functionality is exactly what I hoped for. So if @ph is 👍 I'm good too :-D

Copy link
Copy Markdown
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM, I've looked at the CI this is a metricbeat/kafka docker issue.. nothing that this PR introduce.

@kvch kvch force-pushed the refactoring-libbeat-unify-export-dashboards branch from 6b096de to ee19147 Compare November 26, 2018 14:55
@kvch
Copy link
Copy Markdown
Contributor Author

kvch commented Nov 26, 2018

Added forgotten changelog entry.

@kvch kvch merged commit 1411852 into elastic:master Nov 27, 2018
@kvch kvch added v6.6.0 and removed needs_backport PR is waiting to be backported to other branches. labels Nov 27, 2018
kvch added a commit to kvch/beats that referenced this pull request Nov 27, 2018
The existing `export_dashboards.go` and the command `export dashboards` are unified, using the same code and slightly different logic. Configuration of `export_dashboards` is done using the following command line options: `-kibana`, `-space`, `-output`, `-quiet` and `-index-pattern`. The last flag is an odd one out, because it is part of the script, but its value is never used. So far no one complained, so I did not port it. The default value of that flag is the same in case of both methods.

Configuration the Kibana client of `export dashboard` is read from the config of the Beat. Thus, Kibana-related flags are not part of its CLI.

By default `export dashboard` does not decode the exported dashboard. If flag `-decode` passed to the command, the dashboard is decoded.

export dashboards from a dashboards.yml
```
$ ./filebeat export dashboard -yml path/to/dashboards.yml -decode
$ go run dev-tools/cmd/dashboards/export_dashboards.go -yml path/to/dashboards.yml
```
export a dashboard with an id and print to stdout
```
$ ./filebeat export dashboard -id {uuid} -decode
$ go run dev-tools/cmd/dashboards/export_dashboards.go -dashboard {uuid}
```
(cherry picked from commit 1411852)
kvch added a commit that referenced this pull request Nov 27, 2018
The existing `export_dashboards.go` and the command `export dashboards` are unified, using the same code and slightly different logic. Configuration of `export_dashboards` is done using the following command line options: `-kibana`, `-space`, `-output`, `-quiet` and `-index-pattern`. The last flag is an odd one out, because it is part of the script, but its value is never used. So far no one complained, so I did not port it. The default value of that flag is the same in case of both methods.

Configuration the Kibana client of `export dashboard` is read from the config of the Beat. Thus, Kibana-related flags are not part of its CLI.

By default `export dashboard` does not decode the exported dashboard. If flag `-decode` passed to the command, the dashboard is decoded.

export dashboards from a dashboards.yml
```
$ ./filebeat export dashboard -yml path/to/dashboards.yml -decode
$ go run dev-tools/cmd/dashboards/export_dashboards.go -yml path/to/dashboards.yml
```
export a dashboard with an id and print to stdout
```
$ ./filebeat export dashboard -id {uuid} -decode
$ go run dev-tools/cmd/dashboards/export_dashboards.go -dashboard {uuid}
```
(cherry picked from commit 1411852)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants