Add hopannotation1 to paris traceroute #1047

cristinaleonr · 2022-01-27T03:12:32Z

Notes regarding hop annotations for paris traceroute:

We're using the LogTime as the timestamp in the ID and for the Timestamp field itself. This is the only timestamp that I see in the .paris files and it comes from the filename.
I'm not sure if this is equivalent to the CycleStartTime we adopted as a convention for hopannotation1's timestamp.

Example hop IP, hostname, and ID:

This change is

coveralls · 2022-01-27T03:16:18Z

Pull Request Test Coverage Report for Build 7149

15 of 15 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 64.282%

Totals
Change from base Build 7147:	0.1%
Covered Lines:	3855
Relevant Lines:	5997

💛 - Coveralls

stephen-soltesz

Thank you for these changes.

I think the paris1 hopid construction is incorrect, please see the linked doc.

If the filename timestamp is all we have, then that's the right thing. These paris1 hopids only needs the date portion.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @cristinaleonr)

parser/pt.go, line 404 at r2 (raw file):

			}
			// For hopannotation1 hopIDs, the formatting convention is
			// Timestamp_IP_Hostname. However, the Hops initialized above

This description does not match my understanding from the TRC hopid convention. Can you confirm?

parser/pt.go, line 409 at r2 (raw file):

			// The same convention will have to be adopted in the paris1
			// parser, so its hops can be joined with the synthetic ones.
			hopID := GetHopID(float64(logTime.UTC().Unix()), source.IP, source.IP)

source.IP, source.IP does not look correct, please see comment below.

parser/pt.go, line 425 at r2 (raw file):

				Hostname: allNodes[i].parent_hostname,
			}
			hopID := GetHopID(float64(logTime.UTC().Unix()), source.Hostname, source.IP)

Are these the correct three fields composing the hop id from traceroute-caller?
https://docs.google.com/document/d/1VD5t4zRFlYsq_CAH6oCfjynKaEDnpwHnnFbk2mL7_yI/edit?resourcekey=0-NpaXaG91iS8nb02o9fqgbQ#heading=h.mzu1y33z63fh

Also, FYI: @SaiedKazemi (I don't want to miss anything)

parser/pt_test.go, line 367 at r2 (raw file):

			Hostname:    "sr05-te1-8.nuq04.net.google.com",
			HopAnnotation1: &hopannotation.HopAnnotation1{
				ID:        "20170320_sr05-te1-8.nuq04.net.google.com_64.233.174.109",

Please confirm the hop id is correct.

SaiedKazemi · 2022-01-31T20:33:05Z

parser/pt.go, line 425 at r2 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Are these the correct three fields composing the hop id from traceroute-caller?
https://docs.google.com/document/d/1VD5t4zRFlYsq_CAH6oCfjynKaEDnpwHnnFbk2mL7_yI/edit?resourcekey=0-NpaXaG91iS8nb02o9fqgbQ#heading=h.mzu1y33z63fh

Also, FYI: @SaiedKazemi (I don't want to miss anything)

Looks good to me. Here's what the doc says:

The hop annotation file name should consist of the following three fields concatenated with an underscore character (_) and followed by a .json suffix:

  1. Timestamp - This should be the timestamp when traceroute ran (see Hop Annotation File Content for more details).
  2. Hostname - This should be the hostname where TRC is running on.
  3. IP address - This should be the IP address of the hop that is annotated.

stephen-soltesz

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @cristinaleonr and @SaiedKazemi)

parser/pt.go, line 425 at r2 (raw file):

Previously, SaiedKazemi (Saied Kazemi) wrote…

Looks good to me. Here's what the doc says:

The hop annotation file name should consist of the following three fields concatenated with an underscore character (_) and followed by a .json suffix:

  1. Timestamp - This should be the timestamp when traceroute ran (see Hop Annotation File Content for more details).
  2. Hostname - This should be the hostname where TRC is running on.
  3. IP address - This should be the IP address of the hop that is annotated.

@SaiedKazemi should the Hostname be that of the M-Lab server or the reverse-lookup hostname of the hop router? Which is being used here?

SaiedKazemi · 2022-02-01T00:00:37Z

parser/pt.go, line 425 at r2 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

@SaiedKazemi should the Hostname be that of the M-Lab server or the reverse-lookup hostname of the hop router? Which is being used here?

@stephen-soltesz Per our offline conversation, the hostname in the screenshot ge-11-0-2... is not what we want. I will set up a meeting with @cristinaleonr to discuss this.

cristinaleonr

Updated the hopid to use the M-Lab server's Hostname.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @cristinaleonr)

stephen-soltesz

Thank you - I think it can be a little simpler. PTAL?

Reviewed 1 of 1 files at r5.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @cristinaleonr)

etl/globals.go, line 206 at r5 (raw file):

// For example, for 20170129T000000Z-mlab1-lax05-paris-traceroute-0000.tgz,
// it returns mlab1-lax05.
func GetHostname(rawFilename string) string {

I think we can eliminate this in favor of using the result already available from etl.ValidateTestPath in the pt parser.

parser/pt.go, line 925 at r5 (raw file):

	}

	hostname := etl.GetHostname(fileName)

Since the dual use of "hostname" was the source of some of the ambiguity, what do you think of calling this the "machine" throughout instead?

parser/pt.go, line 925 at r5 (raw file):

	}

	hostname := etl.GetHostname(fileName)

Rather than reparsing the archive url, consider reusing the DataPath type returned by etl.ValidateTestPath from the logic where Parse() is called.

The DataPath type has fields for the "Host" and "Site" which together create the machine name.

cristinaleonr

Updated, thanks!

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @cristinaleonr and @stephen-soltesz)

parser/pt.go, line 925 at r5 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Since the dual use of "hostname" was the source of some of the ambiguity, what do you think of calling this the "machine" throughout instead?

Done.

parser/pt.go, line 925 at r5 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Rather than reparsing the archive url, consider reusing the DataPath type returned by etl.ValidateTestPath from the logic where Parse() is called.

The DataPath type has fields for the "Host" and "Site" which together create the machine name.

Done.

stephen-soltesz

Bonus points for updating the screenshot. (and thank you! -- it can be very confusing looking back and PRs when the original description becomes out of sync with what was actually done).

Just to confirm, the HopAnnotations1 are now being populated? Is it the case that previously that PTTest.AnnotateHops() wasn't filling them in b/c the structs were nil?

I think this will lgtm with confirmation.

Reviewed 2 of 3 files at r6, 1 of 2 files at r7, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @cristinaleonr)

parser/pt.go, line 403 at r7 (raw file):

				IP: server_IP,
			}
			hopID := GetHopID(float64(logTime.UTC().Unix()), machine, source.IP)

These lines look identical here and below.

Is it possible/preferable to eliminate the redundancy?

Code quote:

			hopID := GetHopID(float64(logTime.UTC().Unix()), machine, source.IP)
			source.HopAnnotation1 = &hopannotation.HopAnnotation1{
				ID:        hopID,
				Timestamp: logTime,
			}

cristinaleonr

Yes, the HopAnnotations are being populated for paris-traceroute now. And yes, before they weren't before the structs were nil for pt.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @stephen-soltesz)

parser/pt.go, line 403 at r7 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

These lines look identical here and below.

Is it possible/preferable to eliminate the redundancy?

Done.

stephen-soltesz

This - thank you.. DO NOT MERGE YET.

I setup a time for you, @robertodauria and I to talk tomorrow morning (hopefully) to sort out the order of events necessary to minimize headache for getting this into production without breaking anything else and setting us up for decoupling the v1/v2 deployments.

Reviewable status: complete! 1 of 1 approvals obtained

stephen-soltesz · 2022-02-04T15:32:09Z

Part of #1050

Add hopannotation1 to paris traceroute

33f7440

cristinaleonr added 2 commits January 27, 2022 16:15

Update comment

dfb6915

Fix idiom

c8a9816

cristinaleonr requested a review from stephen-soltesz January 27, 2022 16:38

stephen-soltesz requested changes Jan 27, 2022

View reviewed changes

Fix comment

0d7cc83

stephen-soltesz reviewed Jan 31, 2022

View reviewed changes

cristinaleonr added 2 commits February 2, 2022 02:17

Use filepath hostname

368defb

Undo test change

ff22adb

cristinaleonr commented Feb 2, 2022

View reviewed changes

stephen-soltesz requested changes Feb 2, 2022

View reviewed changes

cristinaleonr added 3 commits February 2, 2022 16:43

Use etl.ValidateTestPath()

e575182

Reuse existing Datapath

9c85484

Formatting

c46aade

cristinaleonr commented Feb 2, 2022

View reviewed changes

stephen-soltesz requested changes Feb 2, 2022

View reviewed changes

Refactor code

191866c

cristinaleonr commented Feb 2, 2022

View reviewed changes

stephen-soltesz approved these changes Feb 3, 2022

View reviewed changes

Merge branch 'master' into sandbox-cristinaleon-pt-annotations

a471077

cristinaleonr merged commit 84187df into master Feb 3, 2022

cristinaleonr deleted the sandbox-cristinaleon-pt-annotations branch February 3, 2022 20:51

cristinaleonr added the traceroute label Aug 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hopannotation1 to paris traceroute #1047

Add hopannotation1 to paris traceroute #1047

cristinaleonr commented Jan 27, 2022 •

edited

Loading

coveralls commented Jan 27, 2022 •

edited

Loading

stephen-soltesz left a comment

SaiedKazemi commented Jan 31, 2022

stephen-soltesz left a comment

SaiedKazemi commented Feb 1, 2022

cristinaleonr left a comment

stephen-soltesz left a comment

cristinaleonr left a comment

stephen-soltesz left a comment

cristinaleonr left a comment

stephen-soltesz left a comment

stephen-soltesz commented Feb 4, 2022

Add hopannotation1 to paris traceroute #1047

Add hopannotation1 to paris traceroute #1047

Conversation

cristinaleonr commented Jan 27, 2022 • edited Loading

coveralls commented Jan 27, 2022 • edited Loading

Pull Request Test Coverage Report for Build 7149

💛 - Coveralls

stephen-soltesz left a comment

Choose a reason for hiding this comment

SaiedKazemi commented Jan 31, 2022

stephen-soltesz left a comment

Choose a reason for hiding this comment

SaiedKazemi commented Feb 1, 2022

cristinaleonr left a comment

Choose a reason for hiding this comment

stephen-soltesz left a comment

Choose a reason for hiding this comment

cristinaleonr left a comment

Choose a reason for hiding this comment

stephen-soltesz left a comment

Choose a reason for hiding this comment

cristinaleonr left a comment

Choose a reason for hiding this comment

stephen-soltesz left a comment

Choose a reason for hiding this comment

stephen-soltesz commented Feb 4, 2022

cristinaleonr commented Jan 27, 2022 •

edited

Loading

coveralls commented Jan 27, 2022 •

edited

Loading