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

[Auditbeat] Update host metricset #9421

Merged
merged 18 commits into from Dec 11, 2018

Conversation

cwurm
Copy link
Contributor

@cwurm cwurm commented Dec 6, 2018

Similar to #9139, this updates the host metricset to be in line with the other metricsets in the system module.

High-level changes:

  1. Adds regular state reporting based on state.period/host.state.period
  2. Persists state between restarts in beat.db
  3. Detects changes in host information, with event.action:
    3.1 host_id_changed (when system.host.id changes)
    3.2 reboot (when system.host.boottime changes)
    3.3 host_changed (for all other changes, e.g. hostname, IPs)
  4. Changes to using system.host.ip/system.host.mac instead of system.host.network.interfaces - the add_host_metadata processor reports IPs and MACs only, too, and I don't see value at the moment of reporting the full network information. We can always add it later.

In contrast to the other metricsets, this one maintains its own system.host namespace instead of using the top-level host. This is because host is filled by the add_host_metadata processor. The processor uses cached values, and so a change event would be accompanied by unchanged data.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/secops

@cwurm cwurm mentioned this pull request Dec 6, 2018
21 tasks
x-pack/auditbeat/module/system/host/config.go Outdated Show resolved Hide resolved
x-pack/auditbeat/module/system/host/host.go Outdated Show resolved Hide resolved
x-pack/auditbeat/module/system/host/host.go Outdated Show resolved Hide resolved
x-pack/auditbeat/module/system/host/host.go Show resolved Hide resolved
mapstr.Delete("uptime")
mapstr.Delete("boottime")
h.WriteString(host.info.Timezone)
h.WriteString(strconv.Itoa(host.info.TimezoneOffsetSec))
Copy link
Member

@andrewkroh andrewkroh Dec 7, 2018

Choose a reason for hiding this comment

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

Rather than converting the number to a string (which requires allocation) I think you can use

binary.Write(h, binary.BigEndian, int32(host.Info.TimezoneOffsetSec))

which will write the bytes directly into the hash. It should not fallback to reflection if you cast from int to int32 (or any sized integer type).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Does it matter if it writes big-endian or little-endian? I see us using both quite heavily in the Beats code.

Copy link
Member

Choose a reason for hiding this comment

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

Endian doesn’t matter. It only matters that it’s hard-coded so that the hash doesn’t change between architectures.

x-pack/auditbeat/module/system/host/host.go Outdated Show resolved Hide resolved
"github.com/elastic/go-sysinfo"
"github.com/elastic/go-sysinfo/types"
)

const (
moduleName = "system"
metricsetName = "host"
Copy link
Member

Choose a reason for hiding this comment

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

@andrewkroh @tsg Should this be audit.host?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm planning to rename the module in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, so the module name will be system.audit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was planning to namespace the fields only:

func init() {
	mb.Registry.MustAddMetricSet(moduleName, metricsetName, New,
		mb.DefaultMetricSet(),
        mb.WithNamespace("system.audit")
	)
}

Copy link
Member

Choose a reason for hiding this comment

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

With #9393 it will become important that event.dataset is unique, meaning it should be system.audit.process here. Let's see what tricks we can apply here.

Copy link
Member

Choose a reason for hiding this comment

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

+1 on getting the PR in and then as soon as #9393 is also merged, will figure it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see how having data from two different metricsets having the same event.dataset can be confusing. But I don't think that is different from having the same event.module and event.metricset.

Still, I could see value in having unique dataset names. Who knows, maybe sometime in the future we might have only datasets, and no more modules/metricsets.

I opened #9499 for namespacing the system module, we can discuss any changes there.

@cwurm cwurm merged commit 9f43eb6 into elastic:feature-auditbeat-host Dec 11, 2018
cwurm pushed a commit to cwurm/beats that referenced this pull request Dec 16, 2018
Updates the `host` metricset to be in line with the other metricsets in the `system` module:

1. Adds regular state reporting based on `state.period`/`host.state.period`
2. Persists state between restarts in `beat.db`
3. Detects changes in host information
4. Changes to using `system.host.ip`/`system.host.mac` instead of `system.host.network.interfaces`
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.

None yet

6 participants