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

Add process.pgid field for processes group id #311

Merged
merged 5 commits into from
May 1, 2019
Merged

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Jan 25, 2019

pgid is an identifier for groups of processes, it is usually the pid of
the leader of the group.

@jsoriano jsoriano added enhancement New feature or request review labels Jan 25, 2019
@jsoriano jsoriano self-assigned this Jan 25, 2019
@webmat
Copy link
Contributor

webmat commented Jan 25, 2019

Hey @jsoriano actually the best way here would be to make group reusable at process.group (and same for reusing user at process.user, for good measure).

This way we'll be able to map very simply to process.user.id and process.group.id, and also process.user.name and process.group.name if we desire.

WDYT?

@MikePaquette
Copy link
Contributor

@webmat I am outside my knowledge base, but have to ask:

  1. Rather than nesting process.group.* couldn't we just populate process.* and group.* at the top level?
  2. Rather than nesting process.user.*couldn't we just populate process.* and user.* at the top level?

I am concerned from an ECS perspective that we'll now have double-nesting of re-usable fields, such as user.*, user.group.*, process.user.*, process.group.*, process.user.group.* which is going to make this hard to understand.

@webmat
Copy link
Contributor

webmat commented Jan 26, 2019

@jsoriano Actually you make a good point. I was conflating your need with another discussion we've been having over here: elastic/beats#10192.

In this other situation, we are displaying all considered sources of information to determine the final / effective user and group ID. So the same event can potentially contain audit/effective/filesystem/saved/object IDs for user and group. In this case, it's necessary to nest everything in a structured manner.

But you're right that if you have an event that describes one process, then user.* and group.* at the top level should be used.

@jsoriano
Copy link
Member Author

Take into account that this pgid doesn't identify a group of users but a group of processes, the kind of value would be the same as pid or ppid. I think it makes more sense in process.pgid.

I am concerned from an ECS perspective that we'll now have double-nesting of re-usable fields, such as user., user.group., process.user., process.group., process.user.group.* which is going to make this hard to understand.

+1, I think we should keep this information at user.*, group.*, if not it can be difficult to corelate different resources of the same users and groups [of users].

@jsoriano
Copy link
Member Author

I have updated the description of the issue to remark that pgid identifies a group of processes.

@jsoriano
Copy link
Member Author

@webmat I have updated the branch, let me know if I should do something else to get this merged.

@webmat
Copy link
Contributor

webmat commented Mar 26, 2019

Thanks @jsoriano. Regular work on improvements to ECS should slowly pick up again next week.

Don't worry, I have this on my todo :-)

@webmat
Copy link
Contributor

webmat commented Apr 25, 2019

@jsoriano Sorry for the time it took me to get back to working on ECS.

@MikePaquette @ruflin This is ready to be merged, and I think it's fine as is. This adds another abbreviation field, which I think is fine, because it's consistent with .pid.

Any objections to me merging this?

@webmat
Copy link
Contributor

webmat commented Apr 30, 2019

Heads up, I'll be merging this tomorrow.

@webmat webmat merged commit 8a2bd9d into elastic:master May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants