Migrate system socket fields metricset to ECS #10339

jsoriano · 2019-01-25T12:48:26Z

No description provided.

metricbeat/helper/socket/listeners.go

webmat

Love where this is going. Left a few comments. Some small things to change.

One of my biggest hopes in life (with regards to this module) would be to get one data.json file for each state we can observe... Hoping the tooling we have for this file allows for this? :-)

metricbeat/helper/socket/listeners.go

webmat · 2019-01-25T19:39:51Z

metricbeat/module/system/socket/_meta/data.json

-            "user": {
-                "id": 1000,
-                "name": "Jaime Soriano Pastor"
+                "cmdline": "/tmp/go-build661675341/b001/socket.test -data"


Are you getting rid of system.socket.local.port?

I agree with keeping system.socket.process.cmdline around, since there's no direct match to it in ECS.

Do you have a data file for incoming or outgoing? Wanted to see the different stats there. You have incoming/outgoing bytes/packets on them, right?

It's kind of odd to keep this here. Could we come up with a name that is not in ECS but still put it under the top level process?

Are you getting rid of system.socket.local.port?

It is moved in this case to server.port as it is a listening socket.

Do you have a data file for incoming or outgoing? Wanted to see the different stats there. You have incoming/outgoing bytes/packets on them, right?

I will try to generate one data.json for each case.

Regarding bytes/packets, I think we don't have this information at this level. I'll take a look but we may need to explicitly enable collection and we would end-up in packetbeat lands. In any case we weren't collecting this before here, we can leave this for a possible future enhancement.

It's kind of odd to keep this here. Could we come up with a name that is not in ECS but still put it under the top level process?

Regarding cmdline, I kept it as is for backwards compatibility, also in the process metricset. I was thinking on removing it because on linux we are just joining args, what is not always accurate, and now we also collect args. But on windows there is a cmdline value in the processes objects.

Maybe cmdline should be in process.cmdline in ECS, and we should rethink how we collect it on Linux.

metricbeat/module/system/socket/socket.go

webmat · 2019-01-25T19:53:48Z

metricbeat/module/system/socket/socket.go

 	}

 	if c.RemotePort != 0 {
+		// Aliases for this are not going to be possible, keeping
+		// duplicated fields by now for backwards comatibility


Why? Do other datasets use it?

I think we should keep BC at least for a time if we cannot offer an alternative with aliases.

Right, the local/remote pair. Gotcha.

Btw, one advantage of local/remote vs source/destination is that you know that local refers to local resources. You can also obtain this info from source/destination, but you need to check direction to know which one is the local one.

webmat · 2019-01-25T20:00:28Z

metricbeat/module/system/socket/socket.go

+				"ip":   c.RemoteIP.String(),
+				"port": c.RemotePort,
+			}
+		}


This made me think to outline the exact mapping I would like to see, in terms of address/ip for the 3 possible states we can observe.

Also made me realize that ECS currently doesn't support sockets that well (noted for later) :-)

listening

local.ip => server.ip

local.port => server.port

incoming

local.ip => server.ip

local.port => server.port

remote.ip => client.ip

remote.port => client.port

outgoing

local.ip => client.ip

local.port => client.port

remote.ip => server.ip

remote.port => server.port

For listening I think we can be quite sure that this is a server, but for incoming/outgoing connections I am not sure we can assume this is a client-server protocol, source/destination doesn't assume protocols.

(Actually local/remote is the terminology used on Linux for this.)

Yes, right now ECS defines the client/server and source/destination pairs. I think there's some contexts where local/remote would also be clearer.

If you're not comfortable using client/server in some of these situations, can you use source/destination? Note that ECS assumes that event sources fill in source/destination all of the time, and client/server only when it makes sense.

So in all the places where you have client/server, you should also duplicate these field sets to source/destination. src/dest are considered the "baseline" that you can always rely on being there.

Yes, using source/destination now, and server only for listening sockets.

I still keep system.socket.local/remote becase we'd need N-N aliases for migration.

metricbeat/module/system/socket/_meta/data.json

ruflin · 2019-01-28T08:40:44Z

dev-tools/ecs-migration.yml

@@ -1211,6 +1211,43 @@

 ## Modules

+### System
+
+- from: system.socket.direction


@simianhacker Could you see that and of the fields change in this file could have an effect on Infra UI?

metricbeat/helper/socket/listeners.go

metricbeat/module/system/socket/_meta/data.json

ruflin · 2019-01-28T08:43:44Z

metricbeat/module/system/socket/_meta/data.json

-            "user": {
-                "id": 1000,
-                "name": "Jaime Soriano Pastor"
+                "cmdline": "/tmp/go-build661675341/b001/socket.test -data"


It's kind of odd to keep this here. Could we come up with a name that is not in ECS but still put it under the top level process?

metricbeat/mb/testing/data_generator.go

Add elastic#6958 also for V2 reporters. When destination path is not a directory, use it as file.

jsoriano · 2019-01-28T12:44:55Z

metricbeat/mb/testing/data_generator.go

@@ -77,29 +77,25 @@ func WriteEventsCond(f mb.EventsFetcher, t testing.TB, cond func(e common.MapStr
 		return fmt.Errorf("no events were generated")


Changes in this file moved to its own PR #10367

webmat

Thanks for doing the work to have more than one data.json. Super helpful here, and will no doubt be helpful in other places too :-)

Still a few details, only two small things which require changes:

Resolve ambiguity around user name (see review)
Please always populate source/destination.

Notes

It's ok to populate client/server only when it makes sense (as long as src/dst are present)
It's ok to keep local/remote around.
No in/out metrics for the sockets: agree this is out of scope. Was only mentioning in case you already had it available.
Ok with error.code. It's in line with current convention.
I'd prefer keeping cmdline where it is. We don't have that defined in ECS, but args is extremely similar. I've taken a note to potentially add it to ECS in the future, but I don't think we should rush it for 7.0.
- If opinions to move are strongly in favour, I could be convinced ;-)

metricbeat/module/system/socket/_meta/fields.yml

metricbeat/module/system/socket/socket.go

webmat · 2019-01-28T18:39:32Z

metricbeat/module/system/socket/socket.go

 	}

 	if c.RemotePort != 0 {
+		// Aliases for this are not going to be possible, keeping
+		// duplicated fields by now for backwards comatibility


Right, the local/remote pair. Gotcha.

webmat

Test failures are legit: this test file needs to be adjusted for the Inbound/Outbound value renames.

After this, I think we're good.

Thanks for filling out source and destination, and for resolving the name ambiguity 👍

jsoriano · 2019-01-31T00:33:28Z

Ouch 🤦‍♂️ variable names in test fixed.

jsoriano · 2019-02-04T12:02:37Z

Waiting for #10516 to update and retest this

jsoriano · 2019-02-04T21:48:32Z

CI failures not related, merging.

Changes requested were addressed

Removes `ecsDirectionString()` from the Auditbeat `socket` dataset following #10339.

jsoriano added module review Metricbeat Metricbeat ecs Team:Integrations Label for the Integrations team breaking change labels Jan 25, 2019

jsoriano self-assigned this Jan 25, 2019

jsoriano requested review from webmat and ruflin January 25, 2019 12:48

jsoriano requested review from a team as code owners January 25, 2019 12:48

jsoriano commented Jan 25, 2019

View reviewed changes

metricbeat/helper/socket/listeners.go Show resolved Hide resolved

webmat reviewed Jan 25, 2019

View reviewed changes

ruflin reviewed Jan 28, 2019

View reviewed changes

houndci-bot reviewed Jan 28, 2019

View reviewed changes

metricbeat/mb/testing/data_generator.go Outdated Show resolved Hide resolved

jsoriano added 7 commits January 28, 2019 13:35

Migrate system socket fields to ECS

81204e1

Add changelog

80c597b

Move comment to review :)

8c07763

Rename incoming/outgoing to inbound/outbound

c211ecc

Format uid as string

5e5eda2

Add event filtering for data generation with V2 reporters

5c31194

Add elastic#6958 also for V2 reporters. When destination path is not a directory, use it as file.

Generate data for inbound and outbound connections too

5027a5b

jsoriano force-pushed the mb-system-socket-ecs branch from 1d44926 to 5027a5b Compare January 28, 2019 12:35

Merge remote-tracking branch 'origin/master' into mb-system-socket-ecs

6725f1e

jsoriano commented Jan 28, 2019

View reviewed changes

jsoriano added 2 commits January 28, 2019 13:49

Remove wrong changelog entry

2c95080

Fix direction mapping

6f36c67

webmat reviewed Jan 28, 2019

View reviewed changes

Collect both name and full name

0d5ebbb

jsoriano added 3 commits January 29, 2019 15:00

Merge remote-tracking branch 'origin/master' into mb-system-socket-ecs

864767e

Fix migration field for full name

1ffc5bb

Remove resolved question

5b45931

webmat previously requested changes Jan 30, 2019

View reviewed changes

Rename incoming/outgoing also in tests

0cc1a92

Merge remote-tracking branch 'origin/master' into mb-system-socket-ecs

c2bdef3

jsoriano force-pushed the mb-system-socket-ecs branch from 456b146 to c2bdef3 Compare February 2, 2019 23:51

Rename incoming/outgoing also on auditbeat

764df33

jsoriano requested a review from a team as a code owner February 3, 2019 00:00

ruflin approved these changes Feb 4, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into mb-system-socket-ecs

7df1ffb

jsoriano merged commit 4095feb into elastic:master Feb 4, 2019

jsoriano deleted the mb-system-socket-ecs branch February 4, 2019 21:50

cwurm mentioned this pull request Feb 6, 2019

[Auditbeat] Socket: Remove ecsDirectionString() #10616

Merged

cwurm pushed a commit that referenced this pull request Feb 7, 2019

[Auditbeat] Socket: Remove ecsDirectionString() (#10616)

2cc1c32

Removes `ecsDirectionString()` from the Auditbeat `socket` dataset following #10339.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate system socket fields metricset to ECS #10339

Migrate system socket fields metricset to ECS #10339

jsoriano commented Jan 25, 2019

webmat left a comment

webmat Jan 25, 2019

webmat Jan 25, 2019

ruflin Jan 28, 2019

jsoriano Jan 28, 2019

webmat Jan 25, 2019

jsoriano Jan 28, 2019

webmat Jan 28, 2019

jsoriano Jan 29, 2019

webmat Jan 25, 2019

jsoriano Jan 28, 2019 •

edited

Loading

webmat Jan 28, 2019

jsoriano Jan 29, 2019

ruflin Jan 28, 2019

ruflin Jan 28, 2019

jsoriano Jan 28, 2019 •

edited

Loading

webmat left a comment •

edited

Loading

webmat Jan 28, 2019

webmat left a comment

jsoriano commented Jan 31, 2019

jsoriano commented Feb 4, 2019

jsoriano commented Feb 4, 2019

		@@ -77,29 +77,25 @@ func WriteEventsCond(f mb.EventsFetcher, t testing.TB, cond func(e common.MapStr
		return fmt.Errorf("no events were generated")

Migrate system socket fields metricset to ECS #10339

Migrate system socket fields metricset to ECS #10339

Conversation

jsoriano commented Jan 25, 2019

webmat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

webmat left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmat left a comment

Choose a reason for hiding this comment

jsoriano commented Jan 31, 2019

jsoriano commented Feb 4, 2019

jsoriano commented Feb 4, 2019

jsoriano Jan 28, 2019 •

edited

Loading

jsoriano Jan 28, 2019 •

edited

Loading

webmat left a comment •

edited

Loading