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

Do not build internal metrics without CGO #6501

Merged
merged 3 commits into from Mar 13, 2018

Conversation

@kvch
Copy link
Contributor

commented Mar 6, 2018

Gosigar only requires CGO for Darwin, FreeBSD and OpenBSD. If someone compiles Beats on these platforms without CGO, metrics are not supported.

Add ephemeral ID to Beats which don't report internal memory usage, etc. So it can still show up in Monitoring UI.

"github.com/elastic/beats/libbeat/logp"
)

var (

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 6, 2018

Collaborator

Could we move the ephemeralID part to a third file which gets compile on all platforms? Like this we can make sure we only have to change it in one place.

@andrewkroh andrewkroh self-requested a review Mar 7, 2018

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

Metrics reporting should work for the Windows binaries where cgo isn't used. The build tags in metrics.go are

// +build darwin linux windows
// +build cgo

which is (darwin OR linux OR windows) AND cgo. But we need to have (darwin AND cgo) OR (freebsd AND cgo) OR linux OR windows. And as a build tag this would become

// +build darwin,cgo freebsd,cgo linux windows

Then over in metrics_other.go you want the build tag to be the exact logical opposite such that the stub is used for all other platforms.

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

I think we must have a test case to verify that xpack monitoring metrics are working correctly in the released binaries. I don't get a strong confidence from evaluating the build tags in my head against how I think they are set during the builds.

A good place to add this would be beats-tester. As you can see on Jenkins we have the log output from each Beat on each platform. The log contains a message like "Total non-zero metrics" that has all the monitoring metrics. We can parse this JSON log message and assert that it contains all of the correct fields as well as the ephemeral_id.

The assertions need to be written twice, once for posix and once for Windows. The two places where it needs added are:

As an example you can use this which collects the line of interest into a variable, parse the data as JSON, then asserts that the object contains particular fields.

@kvch kvch force-pushed the kvch:fix/libbeat/monitoring-build-tags branch from 95af99a to 4d7c8b8 Mar 12, 2018

@kvch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

I updated the branch with build tags I believe to be correct. :) I am still working on adding the assertions to Beats tester. @andrewkroh thanks for the pointers

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2018

I'm wondering if we should merge it as is and then run beats-tester to verify the change or have the beats-tester changes first? I'm good with both options.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2018

@kvch I added the backport label as I assume the change is also needed for 6.2?

@kvch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

Thanks for adding the tag. It's definitely needed in 6.2.

I would wait for beats-tester. I am running it locally right now to validate changes. I would be more comfortable with merging this after it is validated.

@andrewkroh
Copy link
Member

left a comment

I'm happy to help with the beats-tester changes if you want to pair up over Zoom.

@@ -1,5 +1,4 @@
// +build darwin linux windows
// +build cgo
// +build darwin,cgo freebsd,cgo openbsd,cgo linux windows

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 12, 2018

Member

I would leave openbsd off the list because ProcTime and ProcMem are not supported in gosigar.

@@ -1,8 +1,11 @@
// +build !darwin,!linux,!windows darwin,!cgo linux,!cgo windows,!cgo
// +build darwin,!cgo freebsd,!cgo openbsd,!cgo

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 12, 2018

Member

Definitely double check my work, but I think we have a different set of built tags.

From metrics.go (without openbsd):
(darwin AND cgo) OR (freebsd AND cgo) OR linux OR windows

Negating this and simplifying it:

!((darwin AND cgo) OR (freebsd AND cgo) OR linux OR windows)
!(darwin AND cgo) AND !(freebsd AND !cgo) AND !linux AND !windows
(!darwin OR !cgo) AND (!freebsd OR !cgo) AND !linux AND !windows

Conversion to build tags:

// +build !darwin !cgo
// +build !freebsd !cgo
// +build !linux,!windows
@kvch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

I opened a PR for Beats tester: elastic/beats-tester#74

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

I kicked off a beats-tester job with the updated test cases. https://beats-ci.elastic.co/job/elastic+beats-tester+master/172/

We should see it fail until this PR gets merged. 👀

@kvch kvch force-pushed the kvch:fix/libbeat/monitoring-build-tags branch from 8a12185 to 5aea756 Mar 12, 2018

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

We found a secondary problem with the new tests we added. See elastic/beats-tester#74 (comment).

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Mar 13, 2018

Looking at the errors there also seem to be an error related to this change:

17:08:22 TASK [test-beat : Check winlogbeat has monitoring metrics (win)] ***************
17:08:22 fatal: [tester-win12-64]: FAILED! => {"msg": "The conditional check 'log_metrics_event.monitoring.metrics.beat.cpu.system.ticks >= 0' failed. The error was: error while evaluating conditional (log_metrics_event.monitoring.metrics.beat.cpu.system.ticks >= 0): 'dict object' has no attribute 'beat'"}
@kvch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

@ruflin The error you pasted here is expected. Right now on master the registry named "beat" is not added in case of winlogbeat or anything crosscompiled. This PR fixes that problem, because it adds the registry even if system and beat metrics are not supported on a platform.

I would merge this PR now. The other test failures are orthogonal to this PR as @andrewkroh pointed it out. So it should not be blocked.

)

func init() {
beatMetrics := monitoring.Default.NewRegistry("beat")
systemMetrics = monitoring.Default.NewRegistry("system")
ephemeralID = uuid.NewV4()

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 13, 2018

Collaborator

The ephmeralID init is not needed here anymore I think.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Mar 13, 2018

Ok, left a minor comment. Could you also add a CHANGELOG entry?

@kvch kvch force-pushed the kvch:fix/libbeat/monitoring-build-tags branch from 5aea756 to 2e4e18b Mar 13, 2018

@kvch kvch force-pushed the kvch:fix/libbeat/monitoring-build-tags branch from 2e4e18b to e18db5d Mar 13, 2018

@kvch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

Nice catch! Thanks for the review @ruflin
I added a CHANGELOG entry.

@ruflin
ruflin approved these changes Mar 13, 2018
Copy link
Collaborator

left a comment

WFG

@andrewkroh andrewkroh merged commit 11be2ad into elastic:master Mar 13, 2018

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
CLA Commit author is a member of Elasticsearch
Details
beats-ci Build finished.
Details
hound No violations found. Woof!
@andrewkroh

This comment has been minimized.

Copy link
Member

commented Mar 22, 2018

This should be back-ported in case we release a 6.2.4.

@kvch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2018

I am doing that now. :D

kvch added a commit to kvch/beats that referenced this pull request Mar 22, 2018
Do not build internal metrics without CGO (elastic#6501)
* libbeat: build internal metrics on windows and linux without cgo

* libbeat: add ephemeral id even if no internal memory metrics is reported

* fix incorrect build tags && create common file for all platforms

@kvch kvch removed the needs_backport label Mar 22, 2018

kvch added a commit to kvch/beats that referenced this pull request Mar 26, 2018
Do not build internal metrics without CGO (elastic#6501)
* libbeat: build internal metrics on windows and linux without cgo

* libbeat: add ephemeral id even if no internal memory metrics is reported

* fix incorrect build tags && create common file for all platforms
ruflin added a commit that referenced this pull request Mar 27, 2018
Do not build internal metrics without CGO (#6501) (#6633)
* libbeat: build internal metrics on windows and linux without cgo

* libbeat: add ephemeral id even if no internal memory metrics is reported

* fix incorrect build tags && create common file for all platforms

@andrewkroh andrewkroh added the v6.3.0 label Mar 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.