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

New network fields #43

Closed
wants to merge 1 commit into from
Closed

New network fields #43

wants to merge 1 commit into from

Conversation

MikePaquette
Copy link
Contributor

Added total.packets and total.bytes per discussion in PR #2
Added session_id and virtual_ip per discussion in Issue #37
Total of 4 new fields added.

Added total.packets and total.bytes from PR #2

Added session_id and virtual_ip from Issue #37
@MikePaquette MikePaquette requested review from webmat, ruflin and robgil July 11, 2018 19:44
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

I would prefer to have this in multiple PR's so we can get the changes we agree on in quickly. Can you split it up?

Can you add CHANGELOG entries?

- name: session_id
type: keyword
description: >
This is the session ID or connection ID,
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading in this field and the next one about connection, I wonder if we should introduce `connection.* as mentioned in an other thread instead of uptting it under network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @ruflin I forgot to reply before closing this PR out for splitting. Are you just talking about a name change i.e. s/network/connection? The network.* field set is intended to pick up flow and connection-based fields, and also network events that are not flow/connection related. I think this requires a bit more thought before making a decision.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually thinking if we could use a network and a connection prefix. I think a big chunk of the info we have right now in network is in the right place, but there a things like forwarded_ip which probably fit better into connection and also these fields here.

Definitively needs more discussions, just an idea.

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.

Single small request from me.

Not familiar with the discussion around connection yet, but splitting PR in two would make sense to unblock the straightforward changes.

type: long
description: >
Network Total packets: Usually sum (inbound.packets, outbound.packets)
example: 24
Copy link
Contributor

Choose a reason for hiding this comment

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

I would phrase both of these more directly, e.g. "The sum of inbound.packets + outbound.packets", same for bytes.

@MikePaquette
Copy link
Contributor Author

Closing this out to split into two PR's as requested.

@MikePaquette MikePaquette deleted the network-session-id branch July 12, 2018 18:15
@webmat webmat mentioned this pull request Sep 18, 2018
26 tasks
@ruflin ruflin mentioned this pull request Oct 31, 2018
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants