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.

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

add note about overwriting dashboards #6828

Merged
merged 3 commits into from
Jun 20, 2018
Merged

add note about overwriting dashboards #6828

merged 3 commits into from
Jun 20, 2018

Conversation

loekvangool
Copy link

No description provided.

@@ -41,7 +41,9 @@ You can specify the following options in the `setup.dashboards` section of the

If this option is set to true, {beatname_uc} loads the sample Kibana dashboards
automatically on startup. If no other options are set, the dashboard are loaded
from the local `kibana` directory in the home path of the installation.
from the local `kibana` directory in the home path of the installation. Note that
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fairly important, so you might want to call this out as a separate note. I wonder if it might make sense to have a setup.dashboards.overwrite option like we have with the index template?

I'd suggest the following changes (includes a few edits to remove ambiguity in the existing text):

If this option is set to true, {beatname_uc} loads the sample Kibana dashboards
from the local `kibana` directory in the home path of the {beatname_uc} installation.

NOTE: When dashboard loading is enabled, {beatname_uc} overwrites any existing
dashboards that match the names of the dashboards you are loading. This happens
every time {beatname_uc} starts. 

Copy link
Author

Choose a reason for hiding this comment

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

@dedemorton

params.Set("force", "true") //overwrite the existing dashboards
suggests we indeed force overwriting the dashboards (not a developer on Beats, could be wrong about this).

I opened a Discuss on this: https://discuss.elastic.co/t/should-beats-modules-overwrite-kibana-dashboards/127984

I like the new wording. I patched the PR.

@ruflin
Copy link
Member

ruflin commented Apr 17, 2018

@kvch Could you have a look at this if it is our expected behaviour? Could you also have a look at the overwrite config comment? This could be in line with the pipeline loading config?

@@ -40,8 +40,11 @@ You can specify the following options in the `setup.dashboards` section of the
==== `setup.dashboards.enabled`

If this option is set to true, {beatname_uc} loads the sample Kibana dashboards
automatically on startup. If no other options are set, the dashboard are loaded
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the sentence which start with "If no other options are set..." is removed? AFAIK it is still possible to override it using setup.dashboards.directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kvch I think this was my fault! When I suggested an edit to @loekvangool, I inadvertently deleted that line when I copy/pasted the original text. I didn't notice the line was missing because the sentence read correctly without the deleted line. My edit should have said:

If this option is set to true, {beatname_uc} loads the sample Kibana dashboards
automatically on startup. If no other options are set, the dashboard are loaded
from the local `kibana` directory in the home path of the {beatname_uc} installation.

Sorry about the confusion.

@kvch
Copy link
Contributor

kvch commented Apr 17, 2018

@ruflin Dashboards are indeed overwritten every time someone calls setup. This note is valid.
What do you mean by your last question?

@loekvangool
Copy link
Author

Dashboards are indeed overwritten every time someone calls setup

It happens when:

  • setup.dashboards.enabled: true and Beat is restarted
  • {beat} setup --dashboards is called

The first one is the tricky one for me. I myself was running with setup.dashboards.enabled: true for the first time in a new environment because it's easy. I didn't immediate get that it would do this on every subsequent startup and overwrite dashboards.

I reinstated the sentence, thanks for noticing!

@ruflin
Copy link
Member

ruflin commented Apr 19, 2018

@kvch My last comment was referring to your PR here: #6814 Here we also have an overwriting config but this one is off by default. Perhaps we should have dashboards off by default too if they already exist?

@loekvangool
Copy link
Author

@kvch
Copy link
Contributor

kvch commented Apr 20, 2018

The other updating option is disabled by default, because I did not want to change the default behaviour as someone might be building on that. I would go with the current behaviour of dashboard loading now.

In the future it would be nice to decide if loaded object are updated or not and introduce it to all objects regardless where/when it's loaded.

@dedemorton
Copy link
Contributor

jenkins test this please

@dedemorton
Copy link
Contributor

Build failures are unrelated to these changes, so I am merging.

@dedemorton dedemorton added the needs_backport PR is waiting to be backported to other branches. label Jun 20, 2018
@dedemorton dedemorton merged commit b67797b into 6.2 Jun 20, 2018
@dedemorton
Copy link
Contributor

This needs to be backported to 6.3 and forward ported to master.

@dedemorton dedemorton deleted the loekvangool-patch-1 branch June 20, 2018 00:00
dedemorton pushed a commit to dedemorton/beats that referenced this pull request Jun 26, 2018
* add note about overwriting dashboards

* New wording

* Reinstating removed sentence
dedemorton pushed a commit to dedemorton/beats that referenced this pull request Jun 26, 2018
* add note about overwriting dashboards

* New wording

* Reinstating removed sentence
@dedemorton dedemorton removed the needs_backport PR is waiting to be backported to other branches. label Jun 26, 2018
ruflin pushed a commit that referenced this pull request Jun 26, 2018
* add note about overwriting dashboards

* New wording

* Reinstating removed sentence
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
* add note about overwriting dashboards

* New wording

* Reinstating removed sentence
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
* add note about overwriting dashboards

* New wording

* Reinstating removed sentence
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.

4 participants