m-lab / traceroute-caller

A sidecar service which runs traceroute after a connection closes
Apache License 2.0
18 stars 5 forks source link

Add a tool to examine scamper MDA traceroutes #144

Closed SaiedKazemi closed 2 years ago

SaiedKazemi commented 2 years ago

This commit adds a command line tool, called trex, that can do the following on scamper MDA traceroutes:

1. Extract single-path traceroutes from an MDA traceroute.
2. List traceroutes that took longer than a specified duration.
3. List complete and incomplete traceroutes.

This change is Reviewable

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 472


Totals Coverage Status
Change from base Build 463: 0.0%
Covered Lines: 626
Relevant Lines: 646

💛 - Coveralls
SaiedKazemi commented 2 years ago

README.md, line 53 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…
This is great. Thanks!

Glad you find it useful.

SaiedKazemi commented 2 years ago

cmd/trex/main.go, line 74 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…
I find this error check a bit confusing here because it seems like the error corresponds to either the `if` or the `else` conditions (when it only corresponds to the `if`, if I'm not mistaken). Should it go right under line 70?

"err" is defined on line 64 (along with "stat") so its scope is not limited to if/else statements. I will change the code to make it clearer.

SaiedKazemi commented 2 years ago

cmd/trex/main.go, line 104 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…
Should this be set to `math.MaxInt`?

I generally try to minimize the number of packages I import. In this case, the only use of math would be math.MaxInt. Since 1,000,000 seconds is 11 days, far longer than any timeout value we may ever configure for traceroutes, I didn't use math.MaxInt. Does it make sense?

SaiedKazemi commented 2 years ago

cmd/trex/main.go, line 159 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…
Do we already check for this in line 113?

The check in walk() covers the case where we are traversing a directory tree. But the command line allows you to specify individual files (i.e., trex somefile.jsonl). I will remove the check in walk() to have the check in only one place.

SaiedKazemi commented 2 years ago

cmd/trex/main.go, line 186 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…
Should this case increase any of the error counts?

This is an internal error that has nothing to do with the traceroute files. Because we instantiated a new MDA parser, the concrete data type returned by parser.ParseRawData() should be Scamper1 (we need the type switch to "cast" the prsedData interface to Scamper1). Does it make sense?

SaiedKazemi commented 2 years ago

cmd/trex/main.go, line 196 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…
Should we export this since we plan to use it in other packages? If so, we should also add some unit tests. Either way, some description of the algorithm to extract single path traceroutes from multi path would be helpful to have either in the README or as a Docstring.

If we decide to use this function in other software, we need to move it out from main.go to the parser package and then make it public. But we are not sure if we will do that. If we do, then we will also definitely write unit tests for it.

Re the algorithm's description, I will add a comment here and also a paragraph in README.md.

SaiedKazemi commented 2 years ago

@cristinaleonr Thanks a lot for your code review and comments. PTAL.

SaiedKazemi commented 2 years ago

I made the changes as we discussed (actually imported "math"). PTAL.

SaiedKazemi commented 2 years ago

cmd/trex/main.go, line 104 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…
Sure, we don't have to import the package. I was just pointing out that if we're trying to set an initial maximum value, that is the maximum an `int` can be. We can just add a comment as to where the number comes from because otherwise it's hard to tell why we picked it.

I imported "math".

SaiedKazemi commented 2 years ago

cmd/trex/main.go, line 186 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…
Sure. It might make sense to terminate then if this is an internal error because we cannot recover from it and it would cause all the parsing to be unsuccessful.

The program now terminates if this error occurs.