Extract start time, tracelb line, and hops from traceroute output

[SaiedKazemi]

This commit adds the following exported functions to the parser in preparation for adding support for hopannotation1 datatype. It also refactors package testing code to follow Go conventions.

ExtractStartTime()
ExtractTraceLB()
ExtractHops()

The changes were tested via "go test" with 100% coverage and also with "docker-compose" on local workstation.

---

This change is 

[coveralls]

Pull Request Test Coverage Report for Build 348

• 72 of 76 (94.74%) changed or added relevant lines in 3 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-0.9%) to 97.073%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
connectionlistener/connectionlistener.go	11	15	73.33%
<!--	Total:	72	76	94.74%	-->

Files with Coverage Reduction	New Missed Lines	%
tracer/scamper.go	2	92.31%
<!--	Total:	2		-->

Totals
Change from base Build 304:	-0.9%
Covered Lines:	597
Relevant Lines:	615

---

💛 - Coveralls

[SaiedKazemi]

---

parser/json.go, line 1 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

The filename should not be json.go. Can you change it to something more meaningful?

Changed the names to scamper_output.go and scamper_output_test.go because these files parse scamper output.

[SaiedKazemi]

---

parser/json.go, line 87 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

We probably should consider whether these structs even need to be exported.

They don't need to exported.

[SaiedKazemi]

---

parser/json.go, line 131 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

I don't think any of these should be exported. Can you change them, and use export_test.go to make them available for unit tests?

These should be exported because they are called the connectionlistener package. Also, this package can be used as general-purpose scamper output parser. There is no point creating a package that doesn't have any exported functions.

[SaiedKazemi]

---

parser/json.go, line 165 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

In the alternate parser code we were playing with, I separated out the split and 4 line check. Since there is duplicate code here, I suggest you pull out code that separates the 4 lines and puts the raw data into 4 fields in an unexported datatype. Then you can implement these extract functions on that datatype instead of passing in []byte.

I don't quite understand the comment here. ipcache.Trace() returns []byte, which is passed to the parser. Is this an issue?

[SaiedKazemi]

PTAL

[SaiedKazemi]

---

parser/json.go, line 165 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

First, I'd like to point out that this should all be done in the []byte space, without converting to string and back, which causes new allocations. I was thinking that there could be a first step creating a "type jsonl [][]byte", separating the four lines. Then these functions could be methods on that jsonl type object. Everything down to the Unmarshal (in each function) would already be done, in one place, without code duplication. It would also then be clear that you shouldn't be passing any other []byte into these functions, because the methods will have the correct receiver type. I don't think these changes will take more than half an hour, IIUC. Alternatively, since this function is actually parsing EVERYTHING, just create a type that has each of the cycleStart, TracelbLine, and cyclestopLine, parse it all at once, and return the whole object. Otherwise, we have code that does stuff and throws it away, then does it again somewhere else, and it is confusing why it is done in two places. (Not just the split, but also the cyclestart parsing in the current code.)

I see now... thanks for clarifying. I've changed the code to parse the output only once without any conversions from bytes to string or vice versa. PTAL.

[SaiedKazemi]

---

parser/json.go, line 165 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Excellent! Thanks!

Ack'ed.

[SaiedKazemi]

Thanks.

[SaiedKazemi]

---

connectionlistener/connectionlistener.go, line 48 at r3 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Please add a comment here explaining that it is spawning concurrent handling of the entire tracing, postprocessing and file writing. The cache.Trace name isn't very informative.

Done.