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

Jump to bottom

Move HTTP calls to Kibana from New() to Fetch() #15270

Merged
merged 3 commits into from
Dec 31, 2019
Merged

Move HTTP calls to Kibana from New() to Fetch() #15270

merged 3 commits into from
Dec 31, 2019

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Dec 30, 2019 •
edited
Loading

Resolves #15258.

This PR makes the kibana/stats metricset resilient to Kibana's unavailability.

Before this PR, if Kibana was not already running when Metricbeat was started up with the kibana-xpack module enabled, Metricbeat would immediately exit with an error since Kibana was unreachable.

With this PR, Metricbeat will keep running and retrying to reach Kibana periodically. This also allows Kibana to go away temporarily (say, for an upgrade) and keeps Metricbeat running.

Testing this PR

  1. Build Metricbeat with this PR.

    cd $GOPATH/src/github.com/elastic/beats/metricbeat
mage build

  2. Enable the kibana-xpack module.

    ./metricbeat modules enable kibana-xpack

  3. Start up Metricbeat (without starting up Kibana).

    ./metricbeat -e

  4. Note that there are errors in the Metricbeat log about it not being able to connect to Kibana's stats API. But also ensure that Metricbeat keeps running.

  5. Start up Kibana.

  6. Check the Metricbeat log again. Ensure that eventually (< 1 minute), the connection errors go away. You may see 503 errors now; this is expected when Kibana is starting up. Ensure that eventually (< 2 minutes), the 503 errors go away as well.

  7. Ensure that a .monitoring-kibana-*-mb-* index for today's date exists. Ensure that it contains recent (timestamp within last 20 seconds) documents of type:kibana_stats.

  8. Stop Kibana.

  9. Repeat steps 4-7.

@ycombinator
Move HTTP calls to Fetch() from New()
b6e6f48 
This makes the `kibana/stats` metricset resilient to Kibana's unavailability.
@ycombinator ycombinator requested review from a team December 30, 2019 17:48
@elasticmachine
Copy link
Collaborator

Pinging @elastic/stack-monitoring (Stack monitoring)

@ycombinator ycombinator changed the title Move HTTP calls to Kibana from Fetch() from New() Move HTTP calls to Kibana from New() to Fetch() Dec 30, 2019
ph
ph reviewed Dec 30, 2019
View reviewed changes
metricbeat/module/kibana/stats/stats.go
if err != nil {
return nil, err
if m.XPackEnabled {
m.Logger().Error(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ycombinator Do you really need to have a special handling of erros when xpack is enabled? My understanding is that the error will be returned by the Fetch() method and will be logged upstream?

Copy link
Contributor Author

@ycombinator ycombinator Dec 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this change was intentionally made in #12265.

In fact, I'm glad you brought this up — I need to add this same special handling around https://github.com/elastic/beats/pull/15270/files#diff-8710596f0decaae3ca552cedea1eb0a0R80 too!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh good story about the different indices :)

ph
ph approved these changes Dec 30, 2019
View reviewed changes
Copy link
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

@ycombinator
Copy link
Contributor Author

CI failure is related to this PR:

command [go test -cover -coverprofile /tmp/gotestcover-694775026 github.com/elastic/beats/metricbeat/module/kibana/stats]: exit status 1
--- FAIL: TestFetchUsage (0.00s)
    stats_test.go:57: 
        	Error Trace:	stats_test.go:57
        	            				server.go:2007
        	            				server.go:2802
        	            				server.go:1890
        	            				asm_amd64.s:1357
        	Error:      	Not equal: 
        	            	expected: "true"
        	            	actual  : "false"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-true
        	            	+false
        	Test:       	TestFetchUsage
FAIL
coverage: 72.3% of statements
FAIL	github.com/elastic/beats/metricbeat/module/kibana/stats	0.009s

Investigating.

chrisronline
chrisronline approved these changes Dec 31, 2019
View reviewed changes
Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

Functionality LGTM!

@ycombinator ycombinator merged commit d545f69 into elastic:master Dec 31, 2019
@ycombinator ycombinator deleted the mb-kb-resiliency branch December 31, 2019 13:04
This was referenced Dec 31, 2019
Merged
Merged
Merged
Merged
ycombinator added a commit that referenced this pull request Dec 31, 2019
@ycombinator
Move HTTP calls to Kibana from New() to Fetch() (#15270) (#15278)
327b048 
* Move HTTP calls to Fetch() from New()

This makes the `kibana/stats` metricset resilient to Kibana's unavailability.

* Add special handling for errors when xpack.enabled is set to true

* Don't reinit usageLastCollectedOn
ycombinator added a commit that referenced this pull request Dec 31, 2019
@ycombinator
Move HTTP calls to Kibana from New() to Fetch() (#15270) (#15277)
1a91bea 
* Move HTTP calls to Fetch() from New()

This makes the `kibana/stats` metricset resilient to Kibana's unavailability.

* Add special handling for errors when xpack.enabled is set to true

* Don't reinit usageLastCollectedOn
ycombinator added a commit that referenced this pull request Dec 31, 2019
@ycombinator
[6.8] Move HTTP calls to Kibana from New() to Fetch() (#15270) (#15279)
40e65e8 
* Move HTTP calls to Kibana from New() to Fetch() (#15270)

* Move HTTP calls to Fetch() from New()

This makes the `kibana/stats` metricset resilient to Kibana's unavailability.

* Add special handling for errors when xpack.enabled is set to true

* Don't reinit usageLastCollectedOn

* Fixing up backport code to match 6.8 original code
@andresrc andresrc added the Team:Integrations Label for the Integrations team label Mar 6, 2020
@ycombinator ycombinator mentioned this pull request Mar 11, 2020
Merged
4 tasks
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
@ycombinator
Move HTTP calls to Kibana from New() to Fetch() (elastic#15270) (elas…
cd6bd47 
…tic#15278)

* Move HTTP calls to Fetch() from New()

This makes the `kibana/stats` metricset resilient to Kibana's unavailability.

* Add special handling for errors when xpack.enabled is set to true

* Don't reinit usageLastCollectedOn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisronline chrisronline chrisronline approved these changes

@ph ph ph approved these changes

@michalpristas michalpristas Awaiting requested review from michalpristas

Assignees
No one assigned
Labels
bug Feature:Stack Monitoring Metricbeat Metricbeat module Team:Integrations Label for the Integrations team v6.8.7 v7.5.2 v7.6.0 v8.0.0 [zube]: In Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Metricbeat 7.5.1 with kibana-xpack enabled , fails to start if Kibana is unavailable upon start
5 participants
@ycombinator @elasticmachine @ph @chrisronline @andresrc