More Packetbeat cleanups #2972

urso · 2016-11-09T18:29:20Z

unexport private symbols in protocol analyzer and some other modules in packetbeat
remove some unused transaction fields in protocols analyzers

ruflin · 2016-11-10T09:48:08Z

@urso Can you check the test failure before I start reviewing?

- remove unused structure fields - don't export plugin itself - rename plugin implementation to amqpPlugin

- rename HTTP to httpPlugin - unexport all symbols not used by any other package

- rename Mysql to mysqlPlugin - remove some unused structure fields - remove some unecessary conversions

- rename Pgsql to pgsqlPlugin - remove some unused structure fields - unexport private symbols

- unexport private symbols - rename 'Redis' to 'redisPlugin'

- rename Mongodb to mongodbPlugin - remove some unused structure fields - unexport private symbols

- rename Thrift to thriftPlugin - unexport private symbols - remove some unused struct fields

- rename DNS to dnsPlugin - unexport private symbols

- rename ICMP to icmpPlugin - unexport private symbols

tsg · 2016-11-10T13:32:33Z

jenkins, test it

dliappis · 2016-11-10T13:41:10Z

jenkins, test it

dliappis · 2016-11-10T13:42:04Z

jenkins, test it

dliappis · 2016-11-10T13:55:21Z

jenkins, test it

dliappis · 2016-11-10T13:56:19Z

jenkins, test it

elasticsearch-release · 2016-11-10T13:57:41Z

test from build-eu-00

dliappis · 2016-11-10T14:35:45Z

jenkins, test it

ruflin

I went through the changes from top to bottom and mainly tried to spot functional changes. I left comments where I was not 100% sure about the change without going deeper into the code to figure out where these changes come from. As most of the changes I commented appeared several times it seemed like all of these were intentional and I stopped comment even though they appeared more often.

This changes makes most of the varialbes which are not used outside the package private. I'm not 100% sure I agree with this change. The reasoning is that I see packetbeat not only as a "tool" but also as a library. For example if someone builds a new protocol but could use some methods from Http, that was possible before but now because they are private he can't. To change it a contribution is necessary which normally leads to people just copy code over. I'm ok with the changes but I think we should be careful here and think more specific which methods should be exposed.

Is there a specific reasons you renamed MongoDb to MongodbPlugin etc? Was there an issue with the previous naming? Not sure if this addition is needed in the name.

I would appreciate to have one PR per commit in the future which makes the review easier on my eyes ;-)

ruflin · 2016-11-11T07:24:55Z

packetbeat/beater/packetbeat.go

-	Pub         *publish.PacketbeatPublisher
-	Sniff       *sniffer.SnifferSetup
+type packetbeat struct {
+	config      config.Config


Not sure about this change as Packetbeat can also be used as Library and it would be nice to at least have access tot he Config options. I sometimes have that problem with Golang libraries that most things are private and cannot be directly access which leads to coping code.

Well, packetbeat can be used as framework, not as a library. The New function for creating an instances is still available. The packetbeat internals are internal now, so no one messes with them when using packetbeat as framework.

Insteadt of using type beater.Packetbeat, one uses the type beat.Beater as returned by New method.

ruflin · 2016-11-11T07:29:06Z

packetbeat/protos/amqp/amqp.go

 	}

-	trans.ts = client.Ts
-	trans.Ts = client.Ts.UnixNano() / 1000


What happened to this and the following line? Already part of client.ts?

The fields removed have never been read from. They are not part of the event being published or anything => save some bytes and drop unused structure fields.

ruflin · 2016-11-11T07:29:36Z

packetbeat/protos/amqp/amqp.go

 	}

-	trans.ts = server.Ts
-	trans.Ts = server.Ts.UnixNano() / 1000


same. Remove unused fields.

ruflin · 2016-11-11T07:31:31Z

packetbeat/protos/amqp/amqp_parser.go

@@ -83,8 +83,7 @@ func readFrameHeader(data []byte) (ret *AmqpFrame, err bool) {
 		logp.Warn("Missing frame end octet in frame, discarding it")
 		return nil, true
 	}
-	frame.Type = data[0]
-	frame.channel = binary.BigEndian.Uint16(data[1:3])


What happened to this line?

same, remove unused fields.

gotta check removal of frame.Type though.

double checking, frame.Type was not remove. Got confused by diff.

ruflin · 2016-11-11T07:32:36Z

packetbeat/protos/amqp/amqp_structs.go

-	channel uint16
+type amqpFrame struct {
+	Type frameType
+	// channel uint16  (frame channel is currently ignored)


Should we remove it then?

I commented it out because: a) it indirectly reflects the wire format. b) maybe in future this will be exposed?

ruflin · 2016-11-11T07:45:50Z

packetbeat/protos/mongodb/mongodb.go

 		trans.event = requ.event
 		trans.method = requ.method

-		trans.cmdline = requ.CmdlineTuple
-		trans.ts = requ.Ts
-		trans.Ts = int64(trans.ts.UnixNano() / 1000) // transactions have microseconds resolution


As this becomes a pattern, I'm quite sure it is set as a default somewhere ;-)

these fields have been set, but never been included in the event being generated (otherwise compilation would fail).

ruflin · 2016-11-11T07:46:17Z

packetbeat/protos/mongodb/mongodb.go

 	}

 	// fill response
 	if resp != nil {
-		if requ == nil {


Not needed anymore?

the value being set has been remove from trans, as field trans.tuple was unused.

ruflin · 2016-11-11T07:47:32Z

packetbeat/protos/mysql/mysql.go

@@ -489,34 +484,33 @@ func (mysql *Mysql) Parse(pkt *protos.Packet, tcptuple *common.TCPTuple,
 		}
 	}

-	if priv.Data[dir] == nil {
-		priv.Data[dir] = &MysqlStream{
-			tcptuple: tcptuple,


This also becomes a pattern :-)

yep, unused field

ruflin · 2016-11-11T07:48:07Z

packetbeat/protos/mysql/mysql.go

 		mysql.transactions.Put(tuple.Hashable(), trans)
 	}

-	trans.ts = msg.Ts
-	trans.Ts = int64(trans.ts.UnixNano() / 1000) // transactions have microseconds resolution


pattern again

unused fields

ruflin · 2016-11-11T07:51:14Z

packetbeat/protos/pgsql/pgsql.go

-	Src          common.Endpoint
-	Dst          common.Endpoint
-	ResponseTime int32
-	Ts           int64


One more pattern

unused fields

urso · 2016-11-11T13:32:43Z

No logic has been changed in this PR. PR did rename a load of structures/fields/methods unexporting mostly private symbols. Plus only unused fields in protocol analyzer plugins have been removed (saves some bytes + I don't have to wonder about 3 timestamp values anymore). Often these fields have been initialised, but once initialized never be used again. No reads have had to be fixed after removing fields. Before removing I also double-checked with guru for all potential references to removed fields.

This changes makes most of the varialbes which are not used outside the package private. I'm not 100% sure I agree with this change. The reasoning is that I see packetbeat not only as a "tool" but also as a library. For example if someone builds a new protocol but could use some methods from Http, that was possible before but now because they are private he can't. To change it a contribution is necessary which normally leads to people just copy code over. I'm ok with the changes but I think we should be careful here and think more specific which methods should be exposed.

I see packetbeat more as a framework than a library. As I already noted, packetbeat can still be used this way. Also the sniffer, decoder and process watcher are still re-usable. The public API has just been restricted to the most essential use (for good I think).

About the HTTP example, I think it's good it discourages people from importing internals from http plugin. If required, I'd rather see common parser support being extracted/made available in general using a third package instead of importing symbols from/between other plugins.

Is there a specific reasons you renamed MongoDb to MongodbPlugin etc? Was there an issue with the previous naming? Not sure if this addition is needed in the name.

renamed it for dealing with naming clashes

urso added the review label Nov 9, 2016

urso added 19 commits November 10, 2016 14:31

amqp analyzer cleanups:

ba3e4d6

- remove unused structure fields - don't export plugin itself - rename plugin implementation to amqpPlugin

amqp proto analyzer: unexport a number of private types and methods

1367ceb

cassandra proto: unexport private symbols

924f151

http proto: unexport symbols

473790e

- rename HTTP to httpPlugin - unexport all symbols not used by any other package

memcache proto: unexport private symbols

d15c3df

minor mysql proto cleanup

596aaeb

- rename Mysql to mysqlPlugin - remove some unused structure fields - remove some unecessary conversions

mysql protos: unexport private symbols

3f15380

pgsql protos cleanup

e88a455

- rename Pgsql to pgsqlPlugin - remove some unused structure fields - unexport private symbols

Fix rename tCPTuple to tcpTuple

24276e8

redis proto cleanups

0463565

- unexport private symbols - rename 'Redis' to 'redisPlugin'

Fix memcache tests

47f4464

nfs protos: unexport private symbols

40b5219

mongodb protos cleanups

d767b54

- rename Mongodb to mongodbPlugin - remove some unused structure fields - unexport private symbols

thrift proto cleanups:

eeb5100

- rename Thrift to thriftPlugin - unexport private symbols - remove some unused struct fields

dns proto cleanups

35cf69f

- rename DNS to dnsPlugin - unexport private symbols

icmp proto

b5f8905

- rename ICMP to icmpPlugin - unexport private symbols

dns proto unexport more symbols

dd2fd25

packetbeat unexport private symbols in submodules

313e752

fix linux build

85f27e8

urso force-pushed the packetbeat-clean-exports branch from 5350b12 to 85f27e8 Compare November 10, 2016 13:31

Export tags with json fields

cc579ca

Fix procs configuration exports

e97c6d1

ruflin reviewed Nov 11, 2016

View reviewed changes

ruflin merged commit ad0781c into elastic:master Nov 14, 2016

urso deleted the packetbeat-clean-exports branch February 19, 2019 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More Packetbeat cleanups #2972

More Packetbeat cleanups #2972

urso commented Nov 9, 2016

ruflin commented Nov 10, 2016

tsg commented Nov 10, 2016

dliappis commented Nov 10, 2016

dliappis commented Nov 10, 2016

dliappis commented Nov 10, 2016

dliappis commented Nov 10, 2016

elasticsearch-release commented Nov 10, 2016

dliappis commented Nov 10, 2016

ruflin left a comment

ruflin Nov 11, 2016

urso Nov 11, 2016

ruflin Nov 11, 2016

urso Nov 11, 2016

ruflin Nov 11, 2016

urso Nov 11, 2016

ruflin Nov 11, 2016

urso Nov 11, 2016

urso Nov 11, 2016

urso Nov 11, 2016

ruflin Nov 11, 2016

urso Nov 11, 2016

ruflin Nov 11, 2016

urso Nov 11, 2016

ruflin Nov 11, 2016

urso Nov 11, 2016

ruflin Nov 11, 2016

urso Nov 11, 2016

ruflin Nov 11, 2016

urso Nov 11, 2016

ruflin Nov 11, 2016

urso Nov 11, 2016

urso commented Nov 11, 2016

More Packetbeat cleanups #2972

More Packetbeat cleanups #2972

Conversation

urso commented Nov 9, 2016

ruflin commented Nov 10, 2016

tsg commented Nov 10, 2016

dliappis commented Nov 10, 2016

dliappis commented Nov 10, 2016

dliappis commented Nov 10, 2016

dliappis commented Nov 10, 2016

elasticsearch-release commented Nov 10, 2016

dliappis commented Nov 10, 2016

ruflin 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urso commented Nov 11, 2016