m-lab / traceroute-caller

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

Ignore empty IP addresses and traceroute data #140

Closed SaiedKazemi closed 2 years ago

SaiedKazemi commented 2 years ago

This change is Reviewable

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 454


Changes Missing Coverage Covered Lines Changed/Added Lines %
tracer/scamper.go 2 4 50.0%
<!-- Total: 18 20 90.0% -->
Totals Coverage Status
Change from base Build 449: -0.3%
Covered Lines: 626
Relevant Lines: 646

💛 - Coveralls
SaiedKazemi commented 2 years ago

internal/triggertrace/triggertrace.go, line 103 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…
I like this :+1:

Yup, adding context makes it possible to easily group the lines throughout the logs that are related to the same traceroute. Super helpful for debugging.

SaiedKazemi commented 2 years ago

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

Previously, stephen-soltesz (Stephen Soltesz) wrote…
Under what conditions does `ip.String()` return `*`? I'm not sure -- https://go.dev/play/p/YxHvwkdiWNk If this can be unit tested, all the better.

Thank you for pointing this out. I was too quick to assume ip.String() can return "*". The culprit must be a few lines above hops[node.Addr] = struct{}{} which I hadn't noticed because hops[node.Addr] has never been "*" in my local tests. I have added a check there and have also added a specific unit test.

SaiedKazemi commented 2 years ago

tracer/scamper.go, line 145 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…
Not for this PR - but it could be helpful to have metrics for events like this. At one time I hoped we could have a complete accounting of how every event received from tcpinfo passes through traceroute-caller, either "success"fully or with some specific error like this, e.g. "no-data", "command-error", etc.

I agree. In fact, I would like to revisit all event metrics traceroute-caller emits if we can prioritize and allocate time for it.

SaiedKazemi commented 2 years ago

Thank you.