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

Configuration to allow users to retry loading dashboards #6560

Merged
merged 5 commits into from
Mar 19, 2018

Conversation

jalvz
Copy link
Contributor

@jalvz jalvz commented Mar 15, 2018

... if Kibana not reachable, instead of exiting right away.

@@ -21,13 +23,13 @@ type KibanaLoader struct {
msgOutputter MessageOutputter
}

func NewKibanaLoader(cfg *common.Config, dashboardsConfig *Config, hostname string, msgOutputter MessageOutputter) (*KibanaLoader, error) {
func NewKibanaLoader(ctx context.Context, cfg *common.Config, dashboardsConfig *Config, hostname string, msgOutputter MessageOutputter) (*KibanaLoader, error) {

Choose a reason for hiding this comment

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

exported function NewKibanaLoader should have comment or be unexported

Retry *Retry `config:retry`
}

type Retry struct {

Choose a reason for hiding this comment

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

exported type Retry should have comment or be unexported

@jalvz jalvz force-pushed the retry-load-dashboards branch 2 times, most recently from 2371159 to 4ac7c1a Compare March 15, 2018 14:35
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

That's a nice addition and pretty useful when Kibana is slow to start up. I can already see how I will use that in Demo's. Could you add a changelog?

We recently added also some dashboard tests in some beats, but not sure if we can test it there properly.

}

var defaultConfig = Config{
KibanaIndex: ".kibana",
AlwaysKibana: false,
Retry: &Retry{
Enabled: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the defaults for interval and maximum here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing, what do you think is reasonable?
1 sec/ unlimited?

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM, especially as the feature is off by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 3bd3affaa9242bd7072207a911f1313ead19c30e

@jalvz jalvz force-pushed the retry-load-dashboards branch 2 times, most recently from 4ae6de7 to fdc0308 Compare March 15, 2018 15:05
@jalvz
Copy link
Contributor Author

jalvz commented Mar 15, 2018

added changelog fdc0308f204f20d088b571460a509bae5b2427a5

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Could you add the config options to the config reference file here (https://github.com/elastic/beats/blob/master/libbeat/_meta/config.reference.yml#L639) and run make update in the beats directory?

After that we should be good to go.

@@ -99,3 +99,19 @@ NOTE: This setting only works for Kibana 6.0 and newer.

Force loading of dashboards using the Kibana API without querying Elasticsearch for the version
The default is `false`.

[float]
==== `setup.dashboards.retry.enabled`
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the docs.

@jalvz jalvz force-pushed the retry-load-dashboards branch from fdc0308 to 68d3c51 Compare March 19, 2018 08:26
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

The filebeat error is unrelated.

@@ -1223,6 +1223,17 @@ output.elasticsearch:
# how to install the dashboards by first querying Elasticsearch.
#setup.dashboards.always_kibana: false

# If true and Kibana is not reachable at the time when dashboards are loaded,
# it will retry to reconnect to Kibana instead of exiting with an error.
#setup.dashboards.retry.enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are default values we normally put them into the reference doc, so here it would be #setup.dashboards.retry.enabled: false. Similar for the values below.

[float]
==== `setup.dashboards.retry.enabled`

If this option is set to true, and Kibana is not reachable at the time when dashboards 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.

My comment above made me realise that having a comment about the defaults here would also be great. Some at then of the sentence like. Default is false.

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Thx. Waiting for green.

@ruflin ruflin merged commit 9e2838d into elastic:master Mar 19, 2018
@ruflin
Copy link
Contributor

ruflin commented Mar 19, 2018

Thanks for the addition.

jalvz added a commit to jalvz/beats that referenced this pull request Mar 20, 2018
Configuration to allow users to retry loading dashboards if Kibana is not reachable, instead of exiting right away.
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.

3 participants