Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Metricbeat] Remove make kibana command #12440

Closed
wants to merge 1 commit into from
Closed

[Metricbeat] Remove make kibana command #12440

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions metricbeat/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,6 @@ DOCS_BRANCH=$(shell grep doc-branch ../libbeat/docs/version.asciidoc | cut -c 14

include ${ES_BEATS}/libbeat/scripts/Makefile

# Collects all module dashboards
.PHONY: kibana
kibana:
@rm -rf _meta/kibana.generated
@mkdir -p _meta/kibana.generated
@-cp -pr module/*/_meta/kibana/* _meta/kibana.generated

# Collects all module docs
.PHONY: collect-docs
collect-docs: python-env
Expand Down Expand Up @@ -50,7 +43,7 @@ imports: python-env

# Runs all collection steps and updates afterwards
.PHONY: collect
collect: assets collect-docs configs kibana imports
collect: assets collect-docs configs imports
Copy link
Member

Choose a reason for hiding this comment

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

Is this was still called by collect, I wonder why it still worked? I assume there is an other command (inside mage update)? which overwrites this afterwards?

Copy link
Contributor Author

@sayden sayden Jun 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch. My mistake, I was looking at build folder (which is where they are generated for xpack, instead of _meta/kibana.

The problem arised because few os us have had few internal chats over the last months about not being able to load dashboards because we were using make kibana command directly. Right now, it's not expected that you run make kibana by yourself.

make kibana by itself does nothing right now and leads to confusion. It's only required by update to decode/encode them in this script https://github.com/elastic/beats/blob/master/libbeat/scripts/unpack_dashboards.py which mutates them in place.

So, I'd still like to remove the command because it's not intended to be used directly by the developer. I see 2 options:

  • Modify unpack_dashboards.py so it doesn't mutate files but, instead, writes them into a single destination folder (as required by metricbeat setup)
  • Move the commands of make kibana in this PR into the import-dashboards command. kibana.generated files aren't necessary if you aren't importing dashboards so I don't see why we need them into update command. I found that they are used here https://github.com/elastic/beats/blob/master/libbeat/scripts/Makefile#L360

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

What about option 3: Move this logic to mage update as we have in Filebeat and have then the files under build/kibana? This would mean we can remove it completely here (I hope). Unfortunately don't know how many traps are hidden to get this done.

I'm just worried we try to fix something that should disappear anyways.

Be aware that some of the commands are also required to have the correct content for packaging. So it's not necessarly only import-dashboards that needs the files there.

Also I pretty often run Metricbeat locally with -E setup.dashboards.directory=_meta/kibana.generate to play around with the setup command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good but mage update calls make update in Metricbeat 😄 I have checked and we still need quite a lot of work to have mage update. I just took an overall look at it however.

It would be completely out of the scope of this PR. Maybe we can, at least, echo a message about not using this directly or update the comment on Makefile temporarly? I'm happy to work on mage update command but we need to prioritize it first with the rest of our roadmap 😄

Copy link
Member

Choose a reason for hiding this comment

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

Adding a message SGTM. Sorry I'm pushing back on this change but I'm a bit worried about the effect on packaging and the things we haven't thought of.

Copy link
Member

@andrewkroh andrewkroh Jun 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

FYI: There is a mage target for loading dashboards. It's just not applied to all projects. I only added it to Winlogbeat. Adding

	// mage:import
	_ "github.com/elastic/beats/dev-tools/mage/target/dashboards"

establishes mage dashboards:import and mage dashboards:export targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been looking at importing the dashboards command from mage. It's not trivial but, in any case, it still requires make update command to work and so make kibana too 😅

I think it's better to open a new PR to add mage commands slowly.

About the message, just an echo saying Running "make kibana". This make target is deprecated and will be removed soon. This message should only appear when running "make update". Use "metricbeat setup --dasboards" if you want to import dashboards into Kibana"


# Creates a new metricset. Requires the params MODULE and METRICSET
.PHONY: create-metricset
Expand Down