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

Migrate system process metricset fields to ECS #10332

Merged
merged 21 commits into from
Jan 31, 2019

Conversation

jsoriano
Copy link
Member

No description provided.

"ppid": 40547,
"state": "running",
"username": "ruflin"
"pgid": 2080,
Copy link
Member Author

Choose a reason for hiding this comment

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

It can be weird to find pgid here but the rest of ids in process.*, should we add this to ECS?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. even if it's not in ECS, we can push it under process.*.

@webmat FYI

Copy link
Member Author

Choose a reason for hiding this comment

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

To what fields file should I add this field? To the ECS one even if it is not in ECS yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added it by now to field.ecs.yml and opened a PR in ECS elastic/ecs#311

@jsoriano jsoriano force-pushed the mb-system-process-ecs branch from 5f8dea6 to 81d275d Compare January 24, 2019 19:52
@ruflin ruflin mentioned this pull request Jan 24, 2019
@jsoriano jsoriano added the Team:Integrations Label for the Integrations team label Jan 24, 2019
@jsoriano
Copy link
Member Author

CI failure not related, reproduced locally and it seems related to latest ES snapshot:

{"type":"mapper_parsing_exception","reason":"failed to parse field [suricata.eve.flow.start] of type [date]","caused_by":{"type":"illegal_argument_exception","reason":"failed to parse date field [2018-10-03T14:42:44.613469+0000] with format [strict_date_optional_time||epoch_millis]","caused_by":{"type":"date_time_parse_exception","reason":"Text '2018-10-03T14:42:44.613469+0000' could not be parsed, unparsed text found at index 26"}}

"service": {
"type": "system"
},
"system": {
"process": {
"cmdline": "/var/folders/k3/xlwbcsmj6dd7vjv2tg1d7c_40000gn/T/go-build019306533/b001/process.test -data",
"cgroup": {
"blkio": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice data.json update.

metricbeat/module/system/process/process.go Outdated Show resolved Hide resolved
libbeat/metric/system/process/process.go Show resolved Hide resolved
@jsoriano jsoriano force-pushed the mb-system-process-ecs branch from 5490e3a to 95eb36c Compare January 25, 2019 18:18
@jsoriano jsoriano requested a review from a team as a code owner January 25, 2019 18:58
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Mostly commented about the group ID

@@ -4849,6 +4849,16 @@ type: long
Process parent id.


--

*`process.pgid`*::
Copy link
Contributor

Choose a reason for hiding this comment

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

As proposed in your ECS PR, I would go for process.group.id here (and process.user.id if you have that)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually seeing the full context, I wonder if we shouldn't simply map to the top level fields, like you did for user.name. So:

  • system.process.username => user.name
  • system.process.pgid => group.id

And equivalent mappings, if you have user id and group name.

WDYT @ruflin ?

I'd say unless the events can contain more than one user or group, we shouldn't nest them.

Copy link
Member Author

Choose a reason for hiding this comment

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

As commented in elastic/ecs#311, the pgid identifies a group of processes, not a group of users. It is a value similar to ppid, I think it belongs to process.

],
"executable": "/usr/bin/dockerd-ce",
"name": "dockerd",
"pgid": 2080,
Copy link
Contributor

Choose a reason for hiding this comment

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

And don't forget to leave group ID as text (wherever it ends up)

Copy link
Contributor

Choose a reason for hiding this comment

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

By text I meant keyword datatype, here

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that these ids could be keyword, but this is a value of the same kind as pid or ppid and they are defined as long in ECS 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, I was not familiar with pgid. If the semantics are like PID, this can be left as numeric, then.

So recap: I'm good with the field process.pgid, and I'm good with it remaining numeric 👍

libbeat/_meta/fields.ecs.yml Outdated Show resolved Hide resolved
Cwd: exe.Cwd,
Env: env,
Pid: pid,
Ppid: state.Ppid,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, we also have it as part of the monitoring reporting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think this is also used for beats monitoring.

@jsoriano
Copy link
Member Author

pgid moved to metricbeat only fields

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

I'm good with the pgid as it is now. I wasn't familiar with the concept.

Noticed that cwd migration is missing an alias in fields.yml and ecs-migration.yml. After that, I think we're good :-)

],
"executable": "/usr/bin/dockerd-ce",
"name": "dockerd",
"pgid": 2080,
Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, I was not familiar with pgid. If the semantics are like PID, this can be left as numeric, then.

So recap: I'm good with the field process.pgid, and I'm good with it remaining numeric 👍

domain and is formatted as `domain\username`.
type: alias
path: user.name
migration: true
- name: cwd
Copy link
Contributor

Choose a reason for hiding this comment

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

cwd should be migrated to process.working_directory

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good catch, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

]:
assert key in output
assert key in output, "'%s' not found" % key
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ more verbose failures ;-)

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM

@jsoriano jsoriano merged commit 5f90c08 into elastic:master Jan 31, 2019
@jsoriano jsoriano deleted the mb-system-process-ecs branch January 31, 2019 00:50
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