m-lab / traceroute-caller

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

Support regular traceroutes (scamper2 datatype) #136

Closed SaiedKazemi closed 2 years ago

SaiedKazemi commented 2 years ago

This change is Reviewable

SaiedKazemi commented 2 years ago

@cristinaleonr just FYI...

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 433


Totals Coverage Status
Change from base Build 431: 0.3%
Covered Lines: 617
Relevant Lines: 637

💛 - Coveralls
SaiedKazemi commented 2 years ago

caller.go, line 33 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…
Over time, please help me ensure that these invented terms do not "leak" outside of this tool. As I was writing the sketch of the note for the RFC, I found myself using "standard" rather than "regular".

I am not sure how these words can leak outside TRC but the words "mda" and "regular" are not our inventions.

Re "regular", I wasn't sure what to use because, in addition to "standard", words like "classical" and "traditional" have also been used to mean the same thing. Among these options, I chose "regular" after counting the usage of each word in emails from Matthew and the scamper man page.

That being said, it's OK with me to change "regular" to something else if it's better.

SaiedKazemi commented 2 years ago

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

Previously, stephen-soltesz (Stephen Soltesz) wrote…
What is the difference between `errTracerouteType` and `errTraceType`? Are both used?

errTracerouteType is returned when an unknown traceroute type is provided by the user (i.e., when instantiating a new parser).

errTraceType is returned when the expected traceroute type does not match the value of type in a traceroute file. For example, when the type field in an MDA traceroute is not "tracelb" or when the type field in a regular traceroute is not "trace".

If you think we don't need to distinguish between these two cases (i.e., invalid input by user vs invalid traceroute content), we can use just one error for both cases.

SaiedKazemi commented 2 years ago

Please see my comments.

SaiedKazemi commented 2 years ago

caller.go, line 33 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…
Oh, good. Does the scamper documentation refer to these two modes as "mda" and "regular"? That would address my concerns. If not, then that is what I mean by "invented" - that these terms are introduced by us from outside the scope of the scamper tool. We are adding them. I am just advising caution about doing this with care. Naming things is one of the hardest jobs we have.

The scamper man page does not use "regular" but, as mentioned in my comment, scamper's author (Matthew Luckie) does. For example, here is an excerpt from an email that motivated us to do the current work: I am not at all sure why M-Lab uses MDA traceroute (scamper tracelb) over regular traceroute (scamper trace).. So MDA traceroute and regular traceroute aren't invented by us.

SaiedKazemi commented 2 years ago

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

Previously, stephen-soltesz (Stephen Soltesz) wrote…
The distinction is helpful just not apparent to me. Also, I missed that `errTraceType` is used in scamper1.go ( was only looking in this file). It sounds like `errTracerouteType` is about user input while `errTraceType` is about the generated data. Which of these relate to the `scamperTraceType` flag? Does the naming make that easier or harder to discern?

Here's the flag description that the user sees:

  -scamper.trace-type value
        Specify the type of traceroute (mda or regular) to run. (default regular)

And in case of error, the users sees:

$ ./traceroute-caller -scamper.trace-type bad
invalid value "bad" for flag -scamper.trace-type: "bad" is not a valid option: [mda regular]

In case of an error in generated traceroutes, the logs will show a line like the following:

bad: invalid traceroute type

So the user does not see either errTraceType or errTracerouteType.

If you feel strongly that we should switch these error names or use two different names than these, I am open to your suggestion.