m-lab / traceroute-caller

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

Remove TR3 schema and all related code. #97

Closed gfr10598 closed 3 years ago

gfr10598 commented 3 years ago

TODO: rebase after merging #96

This just rips out code that we do not need for 2.1. Rather than deleting everything, it modifies a bit of the code that decodes the scamper output. Not clear if this will be useful, or if we will rewrite it later.

Also adds comments about the existing code and changes we should consider. Some use // XXX and we might want to address them sooner rather than later, or change them to // TODO, or create issues. Feedback appreciated.

Note that this removes a LOT of dependencies from go.sum.

Testing: adapt unit tests to work with modified hop parser unit tests pass 100% test coverage docker build succeeds


This change is Reviewable

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 169


Totals Coverage Status
Change from base Build 163: 0.4%
Covered Lines: 604
Relevant Lines: 604

💛 - Coveralls
SaiedKazemi commented 3 years ago

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

// InitParserVersion initializes the gParserVersion variable for use by all parsers.
func InitParserVersion() string {
  release, ok := os.LookupEnv("RELEASE_TAG")

I don't think gParserVersion and InitParserVersion() are needed. Can we delete them?

SaiedKazemi commented 3 years ago

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


// Metadata contains the UUID and other metadata provided by the traceroute-caller code.
type Metadata struct {

Metadata is also defined in tracer/tracer.go and there's a comment there saying "// TODO: move this struct to ETL parser.".

SaiedKazemi commented 3 years ago

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

  Type      string  `json:"type"`      // "cycle-start"
  ListName  string  `json:"list_name"` // e.g. "/tmp/scamperctrl:58"
  ID        float64 `json:"id"`        // XXX Integer?

Let's convert this and other integer fields that are defined as float64 to int64.

SaiedKazemi commented 3 years ago

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

// Are these necessary, or should we ignore errors in cycleStart/Stop?

We can leave the extra error checking as is while we're transitioning to the new implementation. Once we have confidence in the new code, we can delete them.

SaiedKazemi commented 3 years ago

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

      t.Errorf("Error in InitParserVersion")
  }
}

We should delete TestInitParserVersion() when we delete InitParserVersion().

SaiedKazemi commented 3 years ago

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

}

func TestInitParserVersionCommit(t *testing.T) {

We should also delete TestInitParserVersionCommit().

SaiedKazemi commented 3 years ago

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


// ExtractTraceLB extracts the traceLB line from scamper JSONL
func ExtractTraceLB(data []byte) (*TracelbLine, error) {

Is this exported function used anywhere (other than for testing)?

SaiedKazemi commented 3 years ago

Per our offline chat, a lot of additional code can be removed when we switch to a better JSON paster like buger/jsonparser.

SaiedKazemi commented 3 years ago

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

// ParseJSONL code in etl/parser/pt.go

// ExtractHops parses tracelb and extract all hop addresses.

nit: s/extract/extracts/