-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Heartbeat] Fix fields generation #21874
Conversation
Fields generation seems to have broken at some time in the past, but we never noticed, because we didn't add fields for quite a while. This fixes this.
Pinging @elastic/uptime (Team:Uptime) |
/test please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summarizing the setup here:
- Only OSS heartbeat declares fields.
- All fields are contained in OSS heartbeat/_meta
- Each build (OSS and X-Pack) generates a fields.yml to include in packages for reference. It's not used by the beat software for anything.
- There is a include/fields.go file generated from the all-in-one fields.yml file to embed the fields data into the beat.
- Any fields.go files are imported somewhere to ensure they are included in the binary.
@@ -25,12 +25,15 @@ import ( | |||
// include all heartbeat specific autodiscovery builders | |||
_ "github.com/elastic/beats/v7/heartbeat/autodiscover/builder/hints" | |||
|
|||
// register default heartbeat monitors | |||
_ "github.com/elastic/beats/v7/heartbeat/monitors/defaults" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package looks broken. In the OSS magefile.go there is a generator for it. It looks like it's supposed to be creating imports for each monitor. I'd remove the package and the generation code if it's unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weird thing is this used to work, and somehow broke at some point. Now, when I run mage update
these lines just get deleted. That said, probably easier to just remove it and make it manual since we don't add monitor types frequently.
heartbeat/scripts/mage/package.go
Outdated
if err != nil { | ||
return err | ||
} | ||
return devtools.GenerateAllInOneFieldsGo() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this is run from x-pack/heartbeat it will create x-pack/heartbeat/include/fields.go, but since there are no new fields being introduced we can skip this part. We'd only want to introduce an x-pack/heartbeat/include/fields.go file if X-Pack heartbeat added new fields. And if this file was added we'd only want it to include the new fields (not any of the libbeat or OSS heartbeat fields) since they are already included on the OSS side an inherited by x-pack.
You could use some license based conditionals like in Winlogbeat
or do the same work in both OSS and X-Pack but write the output to OSS only
devtools.GenerateFieldsGo("fields.yml", devtools.OSSBeatDir("include/fields.go"))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will generate new fields when the synthetics stuff is merged in #21436 , and in fact this PR is a bit of a blocker for that one. That said, if it's easier, I'd rather just include all the fields (including x-pack fields) in the OSS heartbeat and keep things simple. Does that work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with the latter approach you suggested. Thanks for the tips!
heartbeat/docs/fields.asciidoc
Outdated
@@ -9570,7 +9570,7 @@ type: long | |||
|
|||
-- | |||
|
|||
[[exported-fields-socks5]] | |||
[[exported-fields-socks6]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
socks6? Where did that change come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed!
heartbeat/Makefile
Outdated
@@ -13,8 +13,7 @@ collect: imports kibana | |||
# Generate imports for all monitors | |||
.PHONY: imports | |||
imports: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's only used by heartbeat's own collect
target so it can be removed if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, removed
* [Heartbeat] Fix fields generation Fields generation seems to have broken at some time in the past, but we never noticed, because we didn't add fields for quite a while. This fixes this. (cherry picked from commit 5d07b87)
I think that this change affects the packaging build, failures like the following one can be reproduced in master now when building x-pack heartbeat packages:
Seen in #22883 |
Fixes a bug introduced in elastic#21874 where fields.yml generation in x-pack stopped happening.
Fixes a bug introduced in #21874 where fields.yml generation in x-pack stopped happening.
Fixes a bug introduced in elastic#21874 where fields.yml generation in x-pack stopped happening. (cherry picked from commit 78a8625)
Fields generation seems to have broken at some time in the past, but we never noticed, because we didn't add fields for quite a while. This fixes this. I had to centralize all field definitions in the top level
_meta
folder, but I'm fine with this change, in many ways this is actually easier.