m-lab / traceroute-caller

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

Pull out common code in trace functions #114

Closed gfr10598 closed 3 years ago

gfr10598 commented 3 years ago

This is not a strict refactoring, but is not intended to change behavior in any way.

Passes unit tests. We should probably also run it in sandbox manually.


This change is Reviewable

gfr10598 commented 3 years ago

I've pushed it manually to sandbox ndt pods.

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 303


Changes Missing Coverage Covered Lines Changed/Added Lines %
tracer/scamper.go 66 68 97.06%
<!-- Total: 66 68 97.06% -->
Files with Coverage Reduction New Missed Lines %
tracer/scamper.go 4 93.51%
<!-- Total: 4 -->
Totals Coverage Status
Change from base Build 301: -0.9%
Covered Lines: 579
Relevant Lines: 591

💛 - Coveralls
SaiedKazemi commented 3 years ago

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

// called binaries have a much smaller "blast radius".
func (s *Scamper) Trace(conn connection.Connection, t time.Time) (out []byte, err error) {
  // XXX Is this still useful?  Does shx.PipeJob we ever panic?

shx does not panic, so this recover() should never happen. But I suggest we change (correct) error/panic/recovery handling in a separate PR.

SaiedKazemi commented 3 years ago

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

// connection.
func (d *ScamperDaemon) Trace(conn connection.Connection, t time.Time) (out []byte, err error) {
  // XXX Is this still useful?  Does shx.PipeJob we ever panic?

See my previous comment.

SaiedKazemi commented 3 years ago

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

      }
      return buff.Bytes(), err
  } else {

Please remove this unnecessary else.