m-lab / traceroute-caller

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

Move trace result writing from tracer package to triggertracer package #151

Closed stephen-soltesz closed 1 year ago

stephen-soltesz commented 1 year ago

This change is one of two to support anonymizing traceroutes. This change alters the tracer.Scamper package interface to separate the collection of traces (by Trace and CachedTrace) from the writing of traces to a file (by WriteFile). This separation allows the triggertracer package, which currently writes hop annotations, to be responsible for writing all files, including the trace output.

The second change will anonymize the trace data before writing to file.


This change is Reviewable

SaiedKazemi commented 1 year ago

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

  Parser           ParseTracer
  HopAnnotator     AnnotateAndArchiver
  Scamper          TracerWriter

I am not sure if Scamper is a good name because the triggertrace package does not (and should not) care about the tool to use for tracing. Today, it's scamper but tomorrow it could be fast-mda-traceroute (or something else). In short, can you think of a better name for the field name?

SaiedKazemi commented 1 year ago

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

  err = h.Scamper.WriteFile(uuid, traceStartTime, rawData)
  if err != nil {
      log.Printf("writing scamper trace failed for uuid: %s: %v\n", uuid, err)

All error messages intraceroute-caller are consistently of the format 'failed to ...so they can be easily searched for with a single pattern. Can you change this tofailed to write trace for uuidand also removescamper` per my earlier comment?

SaiedKazemi commented 1 year ago

tracer/scamper_test.go line 189 at r1 (raw file):

  err = s.WriteFile(wantUUID, faketime, out)
  if err != nil {
      t.Errorf("WriteFile() error = %v, want nil", err)

nit: s/error//

SaiedKazemi commented 1 year ago

tracer/scamper_test.go line 247 at r1 (raw file):

  b, err := s.CachedTrace(uuid, faketime, cachedTrace)
  if err != nil {
      t.Errorf("CacheTrace() returned error %v, want nil", err)

nit: s/returned error/=/ https://google.github.io/styleguide/go/decisions#level-of-detail

SaiedKazemi commented 1 year ago

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

Previously, stephen-soltesz (Stephen Soltesz) wrote…
Great point. How about `Tracer`?

Tracer can be a good choice but if may be confused with the Tracer interface in ipcache. What do you think about Tracetool (variables of type ipcache.Tracer are called tracetool)?

stephen-soltesz commented 1 year ago

Thank you!

SaiedKazemi commented 1 year ago

Thank you for noting that. We can change it in the next anonymization PR.