Issue 291550043: Traceroute mapping stuff
|
Created:
8 years ago by marcus.falgert Modified:
6 years, 8 months ago Reviewers:
Visibility:
Public. |
Descriptionec2_paris-traceroute.sh:
Updated the script to timeout after 1 second for each probe (the -T flag). Perhaps even 0.5 sec is enough.
icmp_response_dpkt.py:
The "server" which waits for UDP probes and sends back ICMP response. Still not finished, but from my findings it complains about a "procId", which is set to the source port when the UDP probe is sent. Different procIds of the sending probe and receiving probe made the traceroute application discard the probe.
I have not set up my keys for the ec2s yet, but it would be easy to replace the logs/warnings with printf's instead, and then skip out on the debugging at all. It should then be easy to find out exactly where it fails in the whole chain of "checks" (they are not that many).
traceroute_exp.sh and traceroute_exp_2.sh:
just different scripts for running the whole chain of python files described below.
traceroute_gnuplot_mapping_creator.py:
creates a gnuplot script from the traceroute_latencies and traceroute_mappings described below. see the flags!
traceroute_to_latency.py:
creates a latency dataset from the traceroutes alone. useful, since the latencies obvserved from traceroutes can differ from the latencies observed from the udp_ping application. see the flags!
traceroute_mapping.py:
takes a latency dataset and a traceroute dataset and attempts to map certain factors, such as unique paths belonging to a certain latency-level, or unique ips seen only a certain latency-level. see the flags!
traceroute_statistics.py:
simply prints out a bunch of statistics about a traceroute dataset, such as the number of unique paths seen etc.
Patch Set 1 #
Total comments: 133
MessagesTotal messages: 7
I put all the files ive been working on during this last week in this code review. So far, I have not really been successful in creating mappings, i.e. figuring out exactly which paths belong to a certain latency level. While some paths are only seen on certain levels, a lot of them are not. I found out that backwards path does not have to be identical to the path seen in the traceroutes, i.e. the path to the destination. This means that a latency level depends on two paths, and one has to figure out how much latency each individual path contributes to the latency. It makes the whole mapping business a lot harder, since you cannot see the backwards path, and even if you perform traceroutes in the opposite direction at the same time, it does not guarantee that the observed path in that direction is the same path.
Sign in to reply to this message.
see my comments, start from traceroute_to_latency.py https://codereview.appspot.com/291550043/diff/1/misc/traceroute_exp_2.sh File misc/traceroute_exp_2.sh (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_exp_2.sh#newc... misc/traceroute_exp_2.sh:3: python traceroute_to_latency.py --regions $1-$2 --plot_offset 0.0 dont use python ... just add #!/usr/bin/env python at the start of each py file https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py File misc/traceroute_mapping.py (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:59: (lookup, err) = p.communicate() if not using err replace it with _ https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:101: def process_traceroutes(files, dst_ip): similarity to statistics.py https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:160: hops_ip.append(parsed) logically it would make sense to replace parsed with ip at this point https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:343: if __name__ == '__main__': in a big and complex scripts you should add examples of how would you run it. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:346: path_trace = "/home/kirillb/data_sets_nc/DATASET-20.0-AZ-PING-TRACE/trace/" should be argument https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:350: parser.add_argument("--regions_trace", default="orgn-tkyo") arent these suppose to match? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:352: parser.add_argument("--protocol", default="ip") use choices=["ip", "bgp",...] https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:353: parser.add_argument("--filter", default="path") add choices https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:355: # format: latency:YYYY.MM.DD.hh.mm.ss-YYYY.MM.DD.hh.mm.ss to help https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:407: latencies = np.genfromtxt(input_lat, delimiter=' ', usecols=(0, 1), dtype=(float, float)) what are these latencies are for and what is this structure? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:418: str = "Latency level %.2f, " % lat keyword https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:420: if FLAGS.protocol == "ip": lots of code duplication just pass levels_unique[lat][FLAGS.protocol]... https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py File misc/traceroute_statistics.py (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:63: def process_traceroutes(files, dst_ip): this function way too big, consider dividing it into a sub functionality. it's ok to iterate over data twice or 3 times, this is just a statistics we dont care about performance, or just parallelize it. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:67: traceroute_time_arr = traceroute.split('.') next 10 or so lines into a function https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:96: if FLAGS.protocol == "bgp": unnecessary nested ifs https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:110: if hops_ip[-1] != dst_ip: what does this mean? dont you want to report an error here ? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:115: if FLAGS.limit_hop_count == "yes": you can just use FLAGS.target_hops > 0 and len(hops_ip) != FLAGS.target_hops: https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:135: for i, hop in enumerate(hops_ip): you overwrite [i] from outer scope https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:155: for i, dict in enumerate(hop_persistence): dict is a built in type https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:185: if value >= 10: what is this? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:199: file_str = file.read() use file.readlines() instead https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:216: if i >= 100: ?? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:221: def find_folder_name(path): duplication https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:244: parser.add_argument("--interval", default="any") add help line, what are the expected values for interval ? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:245: parser.add_argument("--verbose_output", default="yes") add a short one -v https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:248: # ips seen more than x times will be excluded from the plot add your comments into the help message parser.add_argument("--limit_ip_frequency", action='store_true', help="ips seen more than x times will be excluded from the plot") https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:249: parser.add_argument("--limit_ip_frequency", default="no") not used https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:252: parser.add_argument("--limit_hop_count", default="no") dont use strings, use boolean flags: action='store_true' https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:253: parser.add_argument("--hop_count", type=int, default=15) you don't "limit" as of 0 to 15, you only select 15 hops. consider renaming https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:254: # if more than x number of ips are seen at a hop, excluded that hop completely this comment is not correct, you just add hops to hops_avoid but then you do nothing with it https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:258: parser.add_argument("--limit_ip_prevalence", default="no") not used https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:261: input_trace = path + FLAGS.input_trace os.path.join https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:263: # find the correct folder and destination ip create a module function for the next ~5 lines, it is used 3 times in your files https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:294: for i in range(0, 25): MAX_HOPS = 25 https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:313: sys.stdout.write(str) why do you use sys.stdout.write that doesnt print a new line and then follow by an empty print? why not just print it? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:318: sys.stdout.write(str) str is a python keyword, dont overwrite it, arent your text editor highlights it for you? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:325: if FLAGS.limit_hop_count == "yes": this should be boolean https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py File misc/traceroute_to_latency.py (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:9: def parse_trace_line(trace_line): This function is copied 3 times in your files, clear call for a module https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:9: def parse_trace_line(trace_line): it is a good idea to put an example of a line that you parsing in the comments, so that people have a better idea of what is happening https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:13: for elem in trace_line_parsed: instead of parsing every line, I think it would be easier to check if "MPLS" in trace_line:...style https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:20: elif elem == "*": why dont you just check for "* * *" ? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:25: elif len(elem.split(".")) == 4: this check can be replaced with a small utility function is_str_ip(elem) https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:33: return "N/A" why do you reinvent None? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:35: def array_to_unix_timestamp(arr): This function redefined 4 times, move to a module https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:40: min = int(arr[4]) min is a python function, use minutes https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:50: file = open(input_trace + traceroute, 'r') os.path.join https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:50: file = open(input_trace + traceroute, 'r') use with open() instead, you are never close the file https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:68: dst_ips.append(hops_ip[-1]) why do you remove the last one? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:70: if i >= 100: What does this hardcoded break means? https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:75: def find_folder_name(path): redefinition, move to a library https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:86: if folder_src_region == src_region and folder_dst_region == dst_region: 1. What does this check means? 2. dont use globals with lower case e.g. src_region. It's ok to use global FLAGS or SRC_REGION or just pass it in https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:87: folder_name = folder + "/" os.path.join https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:94: path = "/home/kirillb/data_sets_nc/DATASET-20.0-AZ-PING-TRACE/trace/" can be an argument in FLAGS https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:96: parser.add_argument("--interval", default="any") Can be replaced with a boolean flag, you dont use anything but "any" https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:97: parser.add_argument("--plot_offset", type=float, default=-10.0) it is a good practice to add units to your flags. Offset of what? minutes, samples? Also add a small help message to these flags. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:105: input_trace = path + folder_name always use os.path.join(path, folder_name) instead of + https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:112: if FLAGS.interval != "any": in your flag comments you should mention what is the expected input format for this string and why suddenly you parse it https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:121: for traceroute in files: this block of code is duplicated in traceroute_mapping.py, move into a module. select_files_by_date_range() or something https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:132: file = open(input_trace + traceroute, 'r') redefinition of built in type file https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:148: line_split = line.split(":") how does the "line" look like? can you just make a function get_lat_from_line()?
Sign in to reply to this message.
see my comments, start from traceroute_to_latency.py
Sign in to reply to this message.
https://codereview.appspot.com/291550043/diff/1/misc/traceroute_exp_2.sh File misc/traceroute_exp_2.sh (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_exp_2.sh#newc... misc/traceroute_exp_2.sh:3: python traceroute_to_latency.py --regions $1-$2 --plot_offset 0.0 On 2016/04/28 14:49:10, kirill.sc wrote: > dont use python ... just add #!/usr/bin/env python at the start of each py file ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py File misc/traceroute_mapping.py (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:59: (lookup, err) = p.communicate() On 2016/04/28 14:49:10, kirill.sc wrote: > if not using err replace it with _ ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:101: def process_traceroutes(files, dst_ip): On 2016/04/28 14:49:11, kirill.sc wrote: > similarity to statistics.py i know, i could reuse a lot of code for the different scripts https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:160: hops_ip.append(parsed) On 2016/04/28 14:49:10, kirill.sc wrote: > logically it would make sense to replace parsed with ip at this point ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:343: if __name__ == '__main__': On 2016/04/28 14:49:11, kirill.sc wrote: > in a big and complex scripts you should add examples of how would you run it. agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:346: path_trace = "/home/kirillb/data_sets_nc/DATASET-20.0-AZ-PING-TRACE/trace/" On 2016/04/28 14:49:10, kirill.sc wrote: > should be argument agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:350: parser.add_argument("--regions_trace", default="orgn-tkyo") On 2016/04/28 14:49:11, kirill.sc wrote: > arent these suppose to match? they dont have to. you might want to match the trace A-B with the latency B-A. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:352: parser.add_argument("--protocol", default="ip") On 2016/04/28 14:49:11, kirill.sc wrote: > use choices=["ip", "bgp",...] ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:353: parser.add_argument("--filter", default="path") On 2016/04/28 14:49:11, kirill.sc wrote: > add choices ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:355: # format: latency:YYYY.MM.DD.hh.mm.ss-YYYY.MM.DD.hh.mm.ss On 2016/04/28 14:49:10, kirill.sc wrote: > to help ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:407: latencies = np.genfromtxt(input_lat, delimiter=' ', usecols=(0, 1), dtype=(float, float)) On 2016/04/28 14:49:10, kirill.sc wrote: > what are these latencies are for and what is this structure? the latency trace, which the traceroutes are to be mapped to. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:418: str = "Latency level %.2f, " % lat On 2016/04/28 14:49:10, kirill.sc wrote: > keyword ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:420: if FLAGS.protocol == "ip": On 2016/04/28 14:49:11, kirill.sc wrote: > lots of code duplication just pass levels_unique[lat][FLAGS.protocol]... agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py File misc/traceroute_statistics.py (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:63: def process_traceroutes(files, dst_ip): On 2016/04/28 14:49:12, kirill.sc wrote: > this function way too big, consider dividing it into a sub functionality. > it's ok to iterate over data twice or 3 times, this is just a statistics we dont > care about performance, or just parallelize it. ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:67: traceroute_time_arr = traceroute.split('.') On 2016/04/28 14:49:12, kirill.sc wrote: > next 10 or so lines into a function ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:96: if FLAGS.protocol == "bgp": On 2016/04/28 14:49:11, kirill.sc wrote: > unnecessary nested ifs ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:110: if hops_ip[-1] != dst_ip: On 2016/04/28 14:49:11, kirill.sc wrote: > what does this mean? dont you want to report an error here ? means the traceroute is bad, should probably report when it happens, yes https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:115: if FLAGS.limit_hop_count == "yes": On 2016/04/28 14:49:11, kirill.sc wrote: > you can just use FLAGS.target_hops > 0 and len(hops_ip) != FLAGS.target_hops: ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:135: for i, hop in enumerate(hops_ip): On 2016/04/28 14:49:12, kirill.sc wrote: > you overwrite [i] from outer scope thanks for pointing that out, switching to something else would fix it easily. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:155: for i, dict in enumerate(hop_persistence): On 2016/04/28 14:49:12, kirill.sc wrote: > dict is a built in type i know, just didnt feel like it mattered but i see your point https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:185: if value >= 10: On 2016/04/28 14:49:11, kirill.sc wrote: > what is this? instead of listing every single number above 10, just clump them together. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:199: file_str = file.read() On 2016/04/28 14:49:11, kirill.sc wrote: > use file.readlines() instead ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:216: if i >= 100: On 2016/04/28 14:49:11, kirill.sc wrote: > ?? we should easily be able to infer the correct destination ip after 100 traceroutes. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:221: def find_folder_name(path): On 2016/04/28 14:49:12, kirill.sc wrote: > duplication ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:244: parser.add_argument("--interval", default="any") On 2016/04/28 14:49:11, kirill.sc wrote: > add help line, what are the expected values for interval ? agreed. YYYY.MM.DD.hh.mm.ss-YYYY.MM.DD.hh.mm.ss https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:245: parser.add_argument("--verbose_output", default="yes") On 2016/04/28 14:49:11, kirill.sc wrote: > add a short one -v ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:248: # ips seen more than x times will be excluded from the plot On 2016/04/28 14:49:12, kirill.sc wrote: > add your comments into the help message > parser.add_argument("--limit_ip_frequency", action='store_true', help="ips seen > more than x times will be excluded from the plot") ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:249: parser.add_argument("--limit_ip_frequency", default="no") On 2016/04/28 14:49:11, kirill.sc wrote: > not used yep https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:252: parser.add_argument("--limit_hop_count", default="no") On 2016/04/28 14:49:12, kirill.sc wrote: > dont use strings, use boolean flags: action='store_true' had some issues with boolean values in flags, so i just solved it this way instead https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:253: parser.add_argument("--hop_count", type=int, default=15) On 2016/04/28 14:49:11, kirill.sc wrote: > you don't "limit" as of 0 to 15, you only select 15 hops. consider renaming agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:254: # if more than x number of ips are seen at a hop, excluded that hop completely On 2016/04/28 14:49:11, kirill.sc wrote: > this comment is not correct, you just add hops to hops_avoid but then you do > nothing with it i think i removed that code at one point for some reason. i can easily add it back, it was like 2 lines of code. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:258: parser.add_argument("--limit_ip_prevalence", default="no") On 2016/04/28 14:49:12, kirill.sc wrote: > not used yep https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:261: input_trace = path + FLAGS.input_trace On 2016/04/28 14:49:11, kirill.sc wrote: > os.path.join ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:263: # find the correct folder and destination ip On 2016/04/28 14:49:11, kirill.sc wrote: > create a module function for the next ~5 lines, it is used 3 times in your files agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:294: for i in range(0, 25): On 2016/04/28 14:49:11, kirill.sc wrote: > MAX_HOPS = 25 agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:313: sys.stdout.write(str) On 2016/04/28 14:49:11, kirill.sc wrote: > why do you use sys.stdout.write that doesnt print a new line and then follow by > an empty print? why not just print it? if you print in for loop, you get new line for each loop. i only wanted a newline after the loop finished. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:318: sys.stdout.write(str) On 2016/04/28 14:49:11, kirill.sc wrote: > str is a python keyword, dont overwrite it, arent your text editor highlights it > for you? it does, im was not aware that it mattered. when i coded in java a lot i used it all the time. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:325: if FLAGS.limit_hop_count == "yes": On 2016/04/28 14:49:11, kirill.sc wrote: > this should be boolean agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py File misc/traceroute_to_latency.py (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:9: def parse_trace_line(trace_line): On 2016/04/28 14:49:12, kirill.sc wrote: > it is a good idea to put an example of a line that you parsing in the comments, > so that people have a better idea of what is happening agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:9: def parse_trace_line(trace_line): On 2016/04/28 14:49:13, kirill.sc wrote: > This function is copied 3 times in your files, clear call for a module agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:13: for elem in trace_line_parsed: On 2016/04/28 14:49:12, kirill.sc wrote: > instead of parsing every line, I think it would be easier to check if "MPLS" in > trace_line:...style probably yes, i was not sure how efficient that method of doing it was though. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:20: elif elem == "*": On 2016/04/28 14:49:12, kirill.sc wrote: > why dont you just check for "* * *" ? sometimes we can have * * (valid ip here), if we for example time out for 2/3 hops, but last one succeeds https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:25: elif len(elem.split(".")) == 4: On 2016/04/28 14:49:13, kirill.sc wrote: > this check can be replaced with a small utility function is_str_ip(elem) agreed, i just did it this way cause it seemed good enough for the time being https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:33: return "N/A" On 2016/04/28 14:49:12, kirill.sc wrote: > why do you reinvent None? i was not aware such a thing existed. good to know! https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:35: def array_to_unix_timestamp(arr): On 2016/04/28 14:49:12, kirill.sc wrote: > This function redefined 4 times, move to a module agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:40: min = int(arr[4]) On 2016/04/28 14:49:12, kirill.sc wrote: > min is a python function, use minutes ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:50: file = open(input_trace + traceroute, 'r') On 2016/04/28 14:49:12, kirill.sc wrote: > use with open() instead, you are never close the file ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:50: file = open(input_trace + traceroute, 'r') On 2016/04/28 14:49:12, kirill.sc wrote: > os.path.join ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:68: dst_ips.append(hops_ip[-1]) On 2016/04/28 14:49:13, kirill.sc wrote: > why do you remove the last one? i dont remove, i add it. i simply add all last occuring ips in a lista nd then infer the actual destination ip i want from them (the most commonly seen one) https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:70: if i >= 100: On 2016/04/28 14:49:12, kirill.sc wrote: > What does this hardcoded break means? looking through more than 100 traceroutes in order to infer the destination ip is overkill https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:75: def find_folder_name(path): On 2016/04/28 14:49:12, kirill.sc wrote: > redefinition, move to a library agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:86: if folder_src_region == src_region and folder_dst_region == dst_region: On 2016/04/28 14:49:13, kirill.sc wrote: > 1. What does this check means? > 2. dont use globals with lower case e.g. src_region. It's ok to use global FLAGS > or SRC_REGION or just pass it in it means we found the correnct folder, i.e. the folder containing the data for region1-region2. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:87: folder_name = folder + "/" On 2016/04/28 14:49:12, kirill.sc wrote: > os.path.join ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:94: path = "/home/kirillb/data_sets_nc/DATASET-20.0-AZ-PING-TRACE/trace/" On 2016/04/28 14:49:13, kirill.sc wrote: > can be an argument in FLAGS agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:96: parser.add_argument("--interval", default="any") On 2016/04/28 14:49:13, kirill.sc wrote: > Can be replaced with a boolean flag, you dont use anything but "any" yep https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:97: parser.add_argument("--plot_offset", type=float, default=-10.0) On 2016/04/28 14:49:13, kirill.sc wrote: > it is a good practice to add units to your flags. Offset of what? minutes, > samples? > Also add a small help message to these flags. agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:105: input_trace = path + folder_name On 2016/04/28 14:49:12, kirill.sc wrote: > always use os.path.join(path, folder_name) instead of + ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:112: if FLAGS.interval != "any": On 2016/04/28 14:49:13, kirill.sc wrote: > in your flag comments you should mention what is the expected input format for > this string and why suddenly you parse it agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:121: for traceroute in files: On 2016/04/28 14:49:13, kirill.sc wrote: > this block of code is duplicated in traceroute_mapping.py, move into a module. > select_files_by_date_range() or something agreed https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:132: file = open(input_trace + traceroute, 'r') On 2016/04/28 14:49:13, kirill.sc wrote: > redefinition of built in type file ok https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:148: line_split = line.split(":") On 2016/04/28 14:49:12, kirill.sc wrote: > how does the "line" look like? can you just make a function get_lat_from_line()? > see parse_trace_line for the format. i should probably showcase the format of a traceroute file there though
Sign in to reply to this message.
https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py File misc/traceroute_mapping.py (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:350: parser.add_argument("--regions_trace", default="orgn-tkyo") On 2016/04/28 17:06:35, marcus.falgert wrote: > On 2016/04/28 14:49:11, kirill.sc wrote: > > arent these suppose to match? > > they dont have to. you might want to match the trace A-B with the latency B-A. Acknowledged. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_mapping.py#ne... misc/traceroute_mapping.py:407: latencies = np.genfromtxt(input_lat, delimiter=' ', usecols=(0, 1), dtype=(float, float)) On 2016/04/28 17:06:35, marcus.falgert wrote: > On 2016/04/28 14:49:10, kirill.sc wrote: > > what are these latencies are for and what is this structure? > > the latency trace, which the traceroutes are to be mapped to. Acknowledged. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py File misc/traceroute_statistics.py (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_statistics.py... misc/traceroute_statistics.py:313: sys.stdout.write(str) On 2016/04/28 17:06:36, marcus.falgert wrote: > On 2016/04/28 14:49:11, kirill.sc wrote: > > why do you use sys.stdout.write that doesnt print a new line and then follow > by > > an empty print? why not just print it? > > if you print in for loop, you get new line for each loop. i only wanted a > newline after the loop finished. I see, alternatively, I usually create a string buffer for these purposes and then print it once at the end. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py File misc/traceroute_to_latency.py (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:13: for elem in trace_line_parsed: On 2016/04/28 17:06:38, marcus.falgert wrote: > On 2016/04/28 14:49:12, kirill.sc wrote: > > instead of parsing every line, I think it would be easier to check if "MPLS" > in > > trace_line:...style > > probably yes, i was not sure how efficient that method of doing it was though. In the kind of scripts that we write, readability is more important than performance. A single mistake due to complexity can cost much more than 0.5sec per run. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:20: elif elem == "*": On 2016/04/28 17:06:37, marcus.falgert wrote: > On 2016/04/28 14:49:12, kirill.sc wrote: > > why dont you just check for "* * *" ? > > sometimes we can have * * (valid ip here), if we for example time out for 2/3 > hops, but last one succeeds cool, didnt know https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:25: elif len(elem.split(".")) == 4: On 2016/04/28 17:06:38, marcus.falgert wrote: > On 2016/04/28 14:49:13, kirill.sc wrote: > > this check can be replaced with a small utility function is_str_ip(elem) > > agreed, i just did it this way cause it seemed good enough for the time being Acknowledged. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:73: return max(set(dst_ips), key=dst_ips.count) so u just pick up the destination that is most frequently reached? This code begs for some error checking. Perhaps we can dump reg to public ip mapping to avoid this all together https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:86: if folder_src_region == src_region and folder_dst_region == dst_region: On 2016/04/28 17:06:37, marcus.falgert wrote: > On 2016/04/28 14:49:13, kirill.sc wrote: > > 1. What does this check means? > > 2. dont use globals with lower case e.g. src_region. It's ok to use global > FLAGS > > or SRC_REGION or just pass it in > > it means we found the correnct folder, i.e. the folder containing the data for > region1-region2. right, this code is hard to follow, you action in the next line is not clear, and you passing in a path, while src_regs are global
Sign in to reply to this message.
https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py File misc/traceroute_to_latency.py (right): https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:13: for elem in trace_line_parsed: On 2016/04/28 19:39:18, kirill.sc wrote: > On 2016/04/28 17:06:38, marcus.falgert wrote: > > On 2016/04/28 14:49:12, kirill.sc wrote: > > > instead of parsing every line, I think it would be easier to check if "MPLS" > > in > > > trace_line:...style > > > > probably yes, i was not sure how efficient that method of doing it was though. > > In the kind of scripts that we write, readability is more important than > performance. A single mistake due to complexity can cost much more than 0.5sec > per run. Acknowledged. https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:73: return max(set(dst_ips), key=dst_ips.count) On 2016/04/28 19:39:18, kirill.sc wrote: > so u just pick up the destination that is most frequently reached? > This code begs for some error checking. Perhaps we can dump reg to public ip > mapping to avoid this all together correct. but from my experience looking at the 10000 samples we had, only very few (<5) had invalid destination ip. so this should work fine, but i see your concern https://codereview.appspot.com/291550043/diff/1/misc/traceroute_to_latency.py... misc/traceroute_to_latency.py:86: if folder_src_region == src_region and folder_dst_region == dst_region: On 2016/04/28 19:39:18, kirill.sc wrote: > On 2016/04/28 17:06:37, marcus.falgert wrote: > > On 2016/04/28 14:49:13, kirill.sc wrote: > > > 1. What does this check means? > > > 2. dont use globals with lower case e.g. src_region. It's ok to use global > > FLAGS > > > or SRC_REGION or just pass it in > > > > it means we found the correnct folder, i.e. the folder containing the data for > > region1-region2. > > right, this code is hard to follow, you action in the next line is not clear, > and you passing in a path, while src_regs are global Acknowledged.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
