cmd/scion: add json/yaml format to ping and traceroute #4287
Conversation
bunert
changed the title
There was a problem hiding this comment.
What do we gain by supporting yaml output explicitly here? The existing json output can be consumed by a yaml parser either way.
💯 for adding machine readable output to ping/traceroute though.
Reviewable status: 0 of 11 files reviewed, 8 unresolved discussions (waiting on @bunert)
pkg/snet/path/path.go line 44 at r1 (raw file):
Interface int `json:"interface" yaml:"interface"` IA string `json:"isd_as" yaml:"isd_as"` }
Why not just use snet.PathInterface?
scion/cmd/scion/ping.go line 281 at r1 (raw file):
}, }) pktLoss := pingSummary(stats, remote, time.Since(start), cmdout)
It would be great if this could additionally print min/avg/max/mdev round trip time both in "human" and machine readable output formats. The "human" format should match the format of IP ping.
Create the Statistics struct with calculated loss and RTT values first and adapt the pingSummary function to print the values from this struct.
scion/ping/ping.go line 88 at r1 (raw file):
} type Result struct {
These new types really seem to be specific to the update handler used in the cmd package. Move all of these new types to scion/cmd/scion/ping.go.
scion/ping/ping.go line 110 at r1 (raw file):
// Hex-string representing the paths fingerprint. Fingerprint string `json:"fingerprint" yaml:"fingerprint"` Hops []path.Hop `json:"hops" yaml:"hops"`
Here and for all the tools (in particular also for showpaths); it would be useful to print a "sequence" representation of the path which can be to select the path with the --sequence flag in a next tool invocation. For example, this could look like "sequence": "1-ff00:0:133#5 1-ff00:0:120#2,1 1-ff00:0:110#3".
This could either be in addition or as replacement of the hops array -- even if a consumer wants to process the individual hops in some form, parsing the full sequence representation is quite straight forward.
scion/ping/ping.go line 119 at r1 (raw file):
// The internal UDP/IP underlay address of the SCION router that forwards traffic for this path. NextHop string `json:"next_hop" yaml:"next_hop"`
I'd not show the metadata fields (MTU, Latency, Expiry) here. That's what we have showpaths for, or not?
scion/ping/ping.go line 124 at r1 (raw file):
// PingRun defines model for PingRun. type PingUpdate struct { RoundTripTime string `json:"round_trip_time" yaml:"round_trip_time"`
Perhaps it would be more useful to represent durations as numbers, e.g. duration in millisecond as floats. Processing durations with different units is not super easy.
scion/ping/ping.go line 134 at r1 (raw file):
// Source host address SourceHost string `json:"source_host" yaml:"source_host"` SourceIA string `json:"source_isd_as" yaml:"source_isd_as"`
Combine these two fields into a source address in the <ISD>-<AS>,<Host> form?
scion/ping/ping.go line 137 at r1 (raw file):
// Status describing the outcome of a ping response. State string `json:"state" yaml:"state"`
Use State type and add a String method to it.
oncilla
changed the title
There was a problem hiding this comment.
The additional effort to encode it in yaml is trivial.
I think the benefit of an additional format outweighs this additional effort.
Reviewed 3 of 11 files at r1, 4 of 11 files at r3.
Reviewable status: 7 of 14 files reviewed, 22 unresolved discussions (waiting on @bunert and @matzf)
pkg/snet/path.go line 73 at r3 (raw file):
type PathInterface struct { // ID is the ID of the interface. ID common.IFIDType `json:"interface" yaml:"interface"`
Data representation is not a concern of this library package.
I don't think we should extend this specific type with encoding tags.
pkg/snet/path/epic.go line 50 at r3 (raw file):
} func CloneEICDataplanePath(p *EPIC) *EPIC {
Make this a method on the pointer receiver of EPIC.
But I'm questioning now whether this is even a valid thing to do.
The cloned path should not be used to create actual dataplane packets.
Maybe we could have a private field cloned and return an error in SetPath if the path is cloned.
@matzf thoughts?
pkg/snet/path/epic.go line 51 at r3 (raw file):
func CloneEICDataplanePath(p *EPIC) *EPIC { res := &EPIC{}
If we were to keep this, I would write it like so:
Suggestion:
return &EPIC{
AuthPHVF: append(p.AuthPHVF[:0:0], p.AuthPHVF...)
AuthLHVF: append(p.AuthLHVF [:0:0], p.AuthLHVF...)
SCION: append(p.SCION[:0:0], p.SCION...)
counter: counter
}private/path/pathpol/sequence.go line 299 at r3 (raw file):
// GetSequence constructs the sequence string from snet path // example format: "1-ff00:0:133#0 1-ff00:0:120#2,1 0 0 1-ff00:0:110#0"
The sequence that is being built is always fully qualified.
Thus the example should not have any wild cards.
Also, let's use a "code block" for the example:
https://tip.golang.org/doc/comment
Code quote:
// example format: "1-ff00:0:133#0 1-ff00:0:120#2,1 0 0 1-ff00:0:110#0"scion/cmd/scion/ping.go line 67 at r3 (raw file):
// describes the latency between interface i and i+1. Latency []time.Duration `json:"latency,omitempty" yaml:"latency,omitempty"` LocalIp net.IP `json:"local_ip,omitempty" yaml:"local_ip,omitempty"`
Suggestion:
LocalIPscion/cmd/scion/ping.go line 71 at r3 (raw file):
// The maximum transmission unit in bytes for SCION packets. This represents the protocol data // unit (PDU) of the SCION layer on this path. Mtu int `json:"mtu" yaml:"mtu"`
Suggestion:
MTUscion/cmd/scion/ping.go line 139 at r3 (raw file):
} defer closer() var cmdout io.Writer
I would abstract this a step further.
printf := func(fmt string, ctx ...interface{}) {}
if flags.format == human {
printf := func(fmt string, ctx ...interface{}) {
fmt.Fprintf(cmd.OutOrStdout(), fmt, ctx...)
}
}Furthermore, this code appears in all commands.
Let's extract this into a helper.
Code quote:
var cmdout io.Writer
switch flags.format {
case "human":
cmdout = os.Stdout
case "json", "yaml":
cmdout = io.Discard
default:
return serrors.New("format not supported", "format", flags.format)
}scion/cmd/scion/ping.go line 431 at r3 (raw file):
} // JSON writes the ping result as a json object to the writer.
I think these two methods do not really add value.
Just call the encoders directly and drop the methods here.
scion/cmd/scion/showpaths.go line 86 at r3 (raw file):
} defer closer() var cmdout io.Writer
same comment.
scion/cmd/scion/showpaths.go line 166 at r3 (raw file):
cmd.Flags().StringVar(&flags.tracer, "tracing.agent", "", "Tracing agent address") cmd.Flags().BoolVar(&flags.cfg.Epic, "epic", false, "Enable EPIC.") cmd.Flags().MarkDeprecated("json", "json flag is deprecated, use format flag")
If you only mark it as deprectated, you will also need to assign flgas.format appropriately if the json flag is set.
scion/ping/ping.go line 110 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Here and for all the tools (in particular also for showpaths); it would be useful to print a "sequence" representation of the path which can be to select the path with the
--sequenceflag in a next tool invocation. For example, this could look like"sequence": "1-ff00:0:133#5 1-ff00:0:120#2,1 1-ff00:0:110#3".
This could either be in addition or as replacement of thehopsarray -- even if a consumer wants to process the individual hops in some form, parsing the full sequence representation is quite straight forward.
I like the sequence. I still think we should keep the hops.
Having to parse the sequence in jq for example would be painful.
scion/ping/ping.go line 119 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
I'd not show the metadata fields (MTU, Latency, Expiry) here. That's what we have showpaths for, or not?
👍
scion/ping/ping.go line 134 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Combine these two fields into a source address in the
<ISD>-<AS>,<Host>form?
No strong opinion. But so far we kept them separate because we did not have a "standerdized" encoding.
But with the recent change to snet.UDPAddr, I think we could go for it.
scion/showpaths/showpaths.go line 418 at r3 (raw file):
seq, err := pathpol.GetSequence(path) if err != nil { continue
let's set the value to invalid in case of error.
scion/traceroute/traceroute.go line 60 at r3 (raw file):
} type Result struct {
I don't think this belongs here. It is solely data representation.
Let's move it to the cmd/scion/traceroute.go file.
This also means we can share the Path struct between ping and traceroute and enforce consistency.
scion/traceroute/traceroute.go line 346 at r3 (raw file):
} func (h scmpHandler) Handle(pkt *snet.Packet) error {
I think we should reconsider the approach here.
IMO, this is caused by the Update struct having the wrong address type.
Let's change it to snet.SCIONAddress
That way, we do not need to copy the data path at all, and we have all the required information.
scion/traceroute/traceroute.go line 397 at r3 (raw file):
} // JSON writes the traceroute result as a json object to the writer.
Drop these methods completely
There was a problem hiding this comment.
Reviewable status: 7 of 14 files reviewed, 21 unresolved discussions (waiting on @bunert and @matzf)
scion/cmd/scion/showpaths.go line 166 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
If you only mark it as deprectated, you will also need to assign flgas.format appropriately if the json flag is set.
Never mind. This does the right thing, keep as is.
There was a problem hiding this comment.
There is no overhead by adding yaml support as well, so I thought it would be nice to add it. But I don't really have an opinion on that, we can also remove it and adjust it to just support the --json boolean flag.
Reviewable status: 7 of 14 files reviewed, 21 unresolved discussions (waiting on @matzf and @oncilla)
pkg/snet/path.go line 73 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Data representation is not a concern of this library package.
I don't think we should extend this specific type with encoding tags.
So we add another type as a wrapper with tags or just no tags for this struct in the output?
pkg/snet/path/epic.go line 50 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Make this a method on the pointer receiver of EPIC.
But I'm questioning now whether this is even a valid thing to do.
The cloned path should not be used to create actual dataplane packets.Maybe we could have a private field
clonedand return an error inSetPathif the path is cloned.@matzf thoughts?
When changing the remote struct in the traceroute updates this is not required anymore.
pkg/snet/path/epic.go line 51 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
If we were to keep this, I would write it like so:
Done.
pkg/snet/path/path.go line 44 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Why not just use
snet.PathInterface?
Done.
private/path/pathpol/sequence.go line 299 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
The sequence that is being built is always fully qualified.
Thus the example should not have any wild cards.Also, let's use a "code block" for the example:
https://tip.golang.org/doc/comment
correct that way?
scion/cmd/scion/ping.go line 281 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
It would be great if this could additionally print
min/avg/max/mdevround trip time both in "human" and machine readable output formats. The "human" format should match the format of IPping.
Create the Statistics struct with calculated loss and RTT values first and adapt the pingSummary function to print the values from this struct.
Added it using the same format as the time in each update:
rtt min/avg/max/mdev = 396µs/849µs/1.547ms/501µs
Should I adjust it to match the ip ping format?
rtt min/avg/max/mdev = 0.028/0.057/0.089/0.023 ms
scion/cmd/scion/ping.go line 67 at r3 (raw file):
// describes the latency between interface i and i+1. Latency []time.Duration `json:"latency,omitempty" yaml:"latency,omitempty"` LocalIp net.IP `json:"local_ip,omitempty" yaml:"local_ip,omitempty"`
Done.
scion/cmd/scion/ping.go line 71 at r3 (raw file):
// The maximum transmission unit in bytes for SCION packets. This represents the protocol data // unit (PDU) of the SCION layer on this path. Mtu int `json:"mtu" yaml:"mtu"`
Done.
scion/cmd/scion/ping.go line 139 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
I would abstract this a step further.
printf := func(fmt string, ctx ...interface{}) {} if flags.format == human { printf := func(fmt string, ctx ...interface{}) { fmt.Fprintf(cmd.OutOrStdout(), fmt, ctx...) } }Furthermore, this code appears in all commands.
Let's extract this into a helper.
Done.
scion/cmd/scion/ping.go line 431 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
I think these two methods do not really add value.
Just call the encoders directly and drop the methods here.
Done.
scion/cmd/scion/showpaths.go line 86 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
same comment.
Done.
scion/ping/ping.go line 88 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
These new types really seem to be specific to the update handler used in the
cmdpackage. Move all of these new types toscion/cmd/scion/ping.go.
Done.
scion/ping/ping.go line 110 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
I like the sequence. I still think we should keep the hops.
Having to parse the sequence in jq for example would be painful.
I added the hops_sequence field for it. If we remove the additional path information (MTU, Latency, Expiry) from the ping cmd, I was thinking about removing the hops array as well. So if the user really wants the additional path information, the showpath cmd is the way to go while the ping cmd provides the update information. What do you think?
scion/ping/ping.go line 119 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
👍
Done.
scion/ping/ping.go line 124 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Perhaps it would be more useful to represent durations as numbers, e.g. duration in millisecond as floats. Processing durations with different units is not super easy.
yeah, and the additional statistics use it as time duration anyway.
scion/ping/ping.go line 134 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Combine these two fields into a source address in the
<ISD>-<AS>,<Host>form?
Done.
scion/ping/ping.go line 137 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Use
Statetype and add aStringmethod to it.
Done.
scion/showpaths/showpaths.go line 418 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
let's set the value to
invalidin case of error.
Done.
scion/traceroute/traceroute.go line 60 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
I don't think this belongs here. It is solely data representation.
Let's move it to the cmd/scion/traceroute.go file.This also means we can share the Path struct between ping and traceroute and enforce consistency.
Done. Now we have a general Path struct for the ping cmd while the traceroute cmd embeds the general path struct and adds additional fields to the TraceroutePath struct.
scion/traceroute/traceroute.go line 346 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
I think we should reconsider the approach here.
IMO, this is caused by the Update struct having the wrong address type.
Let's change it to snet.SCIONAddressThat way, we do not need to copy the data path at all, and we have all the required information.
Thanks for pointing this out. Much cleaner now.
scion/traceroute/traceroute.go line 397 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Drop these methods completely
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 15 files reviewed, 21 unresolved discussions (waiting on @matzf and @oncilla)
scion/traceroute/traceroute.go line 346 at r3 (raw file):
Previously, bunert wrote…
Thanks for pointing this out. Much cleaner now.
Not sure about the changes as it is not a pointer anymore. Would be great to check if it is okay if we skip those checks, for example in this function:
func (u Update) empty() bool {
pkg/snet/path.go line 73 at r3 (raw file):
Previously, bunert wrote…
So we add another type as a wrapper with tags or just no tags for this struct in the output?
^ done
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 0 of 15 files reviewed, 15 unresolved discussions (waiting on @bunert and @oncilla)
scion/cmd/scion/ping.go line 281 at r1 (raw file):
Previously, bunert wrote…
Added it using the same format as the time in each update:
rtt min/avg/max/mdev = 396µs/849µs/1.547ms/501µsShould I adjust it to match the
ip pingformat?
rtt min/avg/max/mdev = 0.028/0.057/0.089/0.023 ms
Great!
In my opinion, the formatting with mixed units (milliseconds vs microseconds) makes processing of the results harder, both for humans and machines. Something simple like fmt.Sprintf("%.3fms", float64(v.Nanoseconds())/1e6) would work nicely both here and for the individual ping updates.
Not blocking, this can also be revisited later.
scion/ping/ping.go line 110 at r1 (raw file):
Previously, bunert wrote…
I added the
hops_sequencefield for it. If we remove the additional path information (MTU, Latency, Expiry) from thepingcmd, I was thinking about removing the hops array as well. So if the user really wants the additional path information, the showpath cmd is the way to go while the ping cmd provides the update information. What do you think?
Sounds good to me.
scion/ping/ping.go line 43 at r5 (raw file):
AvgRTT time.Duration `json:"avg_RTT" yaml:"avg_RTT"` MaxRTT time.Duration `json:"max_RTT" yaml:"max_RTT"` MdevRTT time.Duration `json:"mdev_RTT" yaml:"mdev_RTT"`
Having all of this information here would be nice if the ping.Run would actually compute these. As the values are computed outside, it seems a bit odd returning this incomplete struct from ping.Run. I'd move the extended stats (and the encoding flags) to a separate type in the cmd/scion/ping package.
There was a problem hiding this comment.
we can also remove it and adjust it to just support the --json boolean flag.
Even if we would only support json and human encoding, I would still go for --format flag.
you never know what the future holds, and boolean flags are not very flexible/extendible.
In any case. I think we should keep yaml. IMO, it is the more human friendly machine readable format anyway.
Reviewed 2 of 11 files at r3, 12 of 13 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @bunert and @matzf)
private/path/pathpol/sequence.go line 299 at r3 (raw file):
Previously, bunert wrote…
correct that way?
No, no wildcards. All the #0 need to be replaced.
E.g., 1-ff00:0:133#42 1-ff00:0:120#2,1 1-ff00:0:110#21
scion/cmd/scion/common.go line 19 at r5 (raw file):
Fingerprint string `json:"fingerprint" yaml:"fingerprint"` Hops []Hop `json:"hops" yaml:"hops"` Sequence string `json:"hops_sequence" yaml:"hops_sequence"`
We have decided internally to call this hop_pattern, at least in the Anapaya Appliance management API it is called this way.
We feel this is a more accurate name for this.
It has been on our todo list to get this also upstreamed to the scion tools. In light of this, I think we should already use the new name here.
And eventually deprecate the old naming.
@matzf thoughts?
Suggestion:
HopPattern string `json:"hop_pattern" yaml:"hop_pattern"`scion/cmd/scion/common.go line 49 at r5 (raw file):
// getPrintf returns a printf function for the "human" formatting flag and an empty one for machine // readable format flags func getPrintf(outputFlag string, writer io.Writer) (
No need for the printf variable.
Also, never return anything else but the zero value when an error is returned.
Suggestion:
func getPrintf(outputFlag string, writer io.Writer) (
func(format string, ctx ...interface{}),
error,
) {
switch outputFlag {
case "human":
return func(format string, ctx ...interface{}) {
fmt.Fprintf(writer, format, ctx...)
}, nil
case "yaml", "json":
return func(format string, ctx ...interface{}) {}, nil
default:
return nil, serrors.New("format not supported", "format", outputFlag)
}
}scion/cmd/scion/traceroute.go line 52 at r5 (raw file):
// Path defines model for Path. type TraceroutePath struct {
Why is there a specific type fro traceroute?
I would expect this to be exactly the same as ping.
scion/cmd/scion/traceroute.go line 97 at r5 (raw file):
SCMP traceroute packets. 'showpaths' can be instructed to output the paths in a specific format using the \--format flag.
That line seems to be a copy paste mistake.
This is the traceroute tool.
TBH, I don't think this adds value. If you do --help, you will see the format flag anyway.
scion/ping/ping.go line 110 at r1 (raw file):
So if the user really wants the additional path information, the showpath cmd is the way to go while the ping cmd provides the update information. What do you think?
I would rather keep the hops always. IMO, the a remote address without the path is underspecified because in SCION the path information is actually crucial to know where the packet is traveling through. Thus, I also consider the output underspecified if the path is not there. Having to do showpaths just to get the path hops is cumbersome and also prone to race conditions.
For me, this is in a different category then the extended path information like advertised latencies etc..
scion/traceroute/traceroute.go line 49 at r5 (raw file):
func (u Update) empty() bool { return u.Index == 0 && u.Interface == 0 && len(u.RTTs) == 0
you still need to check that remote is of zero value
Suggestion:
return u.Index == 0 && u.Remote == (snet.SCIONAddress{}) && u.Interface == 0 && len(u.RTTs) == 0
There was a problem hiding this comment.
Ok. My view was that as this provides no additional usage options, this is only bloat. This is not a very strong opinion though, so let's go ahead with both yaml and json if you both think it's worth it.
In any case. I think we should keep yaml. IMO, it is the more human friendly machine readable format anyway.
With this, I disagree (but it's a moot point, see above...). Yes, for things like configuration files, I would also choose yaml over json -- here, yaml features like comments and more concise notation are very advantageous.
For machine generated output that is intended for machine consumption, I'd argue that simplicity of processing is more important, and so json as the lower common denominator wins.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @bunert and @oncilla)
scion/cmd/scion/common.go line 19 at r5 (raw file):
Previously, oncilla (Dominik Roos) wrote…
We have decided internally to call this
hop_pattern, at least in the Anapaya Appliance management API it is called this way.
We feel this is a more accurate name for this.It has been on our todo list to get this also upstreamed to the scion tools. In light of this, I think we should already use the new name here.
And eventually deprecate the old naming.@matzf thoughts?
While hop pattern does seem like a good name to me too, I think it's simpler for now to keep the naming consistent. Just sequence, like everywhere else.
scion/ping/ping.go line 110 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
So if the user really wants the additional path information, the showpath cmd is the way to go while the ping cmd provides the update information. What do you think?
I would rather keep the hops always. IMO, the a remote address without the path is underspecified because in SCION the path information is actually crucial to know where the packet is traveling through. Thus, I also consider the output underspecified if the path is not there. Having to do showpaths just to get the path hops is cumbersome and also prone to race conditions.
For me, this is in a different category then the extended path information like advertised latencies etc..
Fair enough, also sounds good to me. :)
There was a problem hiding this comment.
Reviewable status: 8 of 15 files reviewed, 7 unresolved discussions (waiting on @matzf and @oncilla)
private/path/pathpol/sequence.go line 299 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
No, no wildcards. All the #0 need to be replaced.
E.g.,
1-ff00:0:133#42 1-ff00:0:120#2,1 1-ff00:0:110#21
Done.
scion/cmd/scion/common.go line 19 at r5 (raw file):
Previously, matzf (Matthias Frei) wrote…
While hop pattern does seem like a good name to me too, I think it's simpler for now to keep the naming consistent. Just
sequence, like everywhere else.
Done.
scion/cmd/scion/common.go line 49 at r5 (raw file):
Previously, oncilla (Dominik Roos) wrote…
No need for the printf variable.
Also, never return anything else but the zero value when an error is returned.
Done.
scion/cmd/scion/ping.go line 281 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Great!
In my opinion, the formatting with mixed units (milliseconds vs microseconds) makes processing of the results harder, both for humans and machines. Something simple like
fmt.Sprintf("%.3fms", float64(v.Nanoseconds())/1e6)would work nicely both here and for the individual ping updates.
Not blocking, this can also be revisited later.
Added it. I also updated the traceroute update formats to match.
scion/cmd/scion/traceroute.go line 52 at r5 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Why is there a specific type fro traceroute?
I would expect this to be exactly the same as ping.
It includes the additional expiry, latency, and MTU information which I removed from the ping cmd as discussed in the review.
scion/cmd/scion/traceroute.go line 97 at r5 (raw file):
Previously, oncilla (Dominik Roos) wrote…
That line seems to be a copy paste mistake.
This is the traceroute tool.TBH, I don't think this adds value. If you do --help, you will see the format flag anyway.
Agree.
scion/ping/ping.go line 43 at r5 (raw file):
Previously, matzf (Matthias Frei) wrote…
Having all of this information here would be nice if the ping.Run would actually compute these. As the values are computed outside, it seems a bit odd returning this incomplete struct from
ping.Run. I'd move the extended stats (and the encoding flags) to a separate type in thecmd/scion/pingpackage.
Adapted but the json tags for the initial stats struct remain in the ping package, is that okay?
scion/traceroute/traceroute.go line 49 at r5 (raw file):
Previously, oncilla (Dominik Roos) wrote…
you still need to check that remote is of zero value
Done.
There was a problem hiding this comment.
Reviewed 4 of 7 files at r6.
Reviewable status: 12 of 15 files reviewed, 7 unresolved discussions (waiting on @bunert and @matzf)
scion/cmd/scion/common.go line 19 at r5 (raw file):
Previously, bunert wrote…
Done.
in that case the json tag should be sequence IMO
scion/cmd/scion/common.go line 57 at r6 (raw file):
return func(format string, ctx ...interface{}) {} default: return nil
Why do we no longer return an error in case of unsupported format?
AFAICT, you always need to do the nil check now and the API of this function just became a little less obvious than with an explicit error return value.
Code quote:
return nilscion/cmd/scion/ping.go line 56 at r6 (raw file):
ping.Stats MinRTT time.Duration `json:"min_RTT" yaml:"min_RTT"` AvgRTT time.Duration `json:"avg_RTT" yaml:"avg_RTT"`
Suggestion:
MinRTT time.Duration `json:"min_rtt" yaml:"min_rtt"`
AvgRTT time.Duration `json:"avg_rtt" yaml:"avg_rtt"`
MaxRTT time.Duration `json:"max_rtt" yaml:"max_rtt"`
MdevRTT time.Duration `json:"mdev_rtt" yaml:"mdev_rtt"`scion/cmd/scion/ping.go line 363 at r6 (raw file):
} func (s *Stats) pingSummary(
This method does too much. It should only have one single responsibility.
At the moment, it calculates the statistics and prints some output.
This should be split into two individual steps.
scion/cmd/scion/ping.go line 386 at r6 (raw file):
// processRTTs computes the min, average, max and standard deviation of the update RTTs func (s *Stats) processRTTs(replies []PingUpdate) {
ditto
scion/cmd/scion/traceroute.go line 52 at r5 (raw file):
Previously, bunert wrote…
It includes the additional expiry, latency, and MTU information which I removed from the ping cmd as discussed in the review.
Yes, but why do you want these extra fields on the traceroute path?
From my point of view, either both ping and traceroute have them, or none.
The two commands are very similar in their purpose, so I would also expect the paths to contain the exact same amount of infromation.
I think @matzf had in mind that you will adapt both and not just ping.
There was a problem hiding this comment.
Reviewable status: 12 of 15 files reviewed, 7 unresolved discussions (waiting on @bunert and @matzf)
scion/cmd/scion/ping.go line 363 at r6 (raw file):
Previously, oncilla (Dominik Roos) wrote…
This method does too much. It should only have one single responsibility.
At the moment, it calculates the statistics and prints some output.This should be split into two individual steps.
So, IMO. You should have:
res.Statistics = calculateStats(stats, res.Replies)
// ...
func calculateStats(stats ping.Stats, replies []PingUpdate) Stats {
// calculate stats
}
func printStats(stats Stats, printf func(string, ...interface{})) {
// print here
}You might even consider dropping printStats and move that functionality into the switch statement
There was a problem hiding this comment.
Reviewable status: 12 of 15 files reviewed, 6 unresolved discussions (waiting on @bunert and @matzf)
scion/cmd/scion/ping.go line 363 at r6 (raw file):
Previously, oncilla (Dominik Roos) wrote…
So, IMO. You should have:
res.Statistics = calculateStats(stats, res.Replies) // ... func calculateStats(stats ping.Stats, replies []PingUpdate) Stats { // calculate stats } func printStats(stats Stats, printf func(string, ...interface{})) { // print here }You might even consider dropping printStats and move that functionality into the switch statement
(also, for the record. I think making this a method with side effects is surprising.)
There was a problem hiding this comment.
Reviewable status: 11 of 15 files reviewed, 6 unresolved discussions (waiting on @matzf and @oncilla)
scion/cmd/scion/common.go line 19 at r5 (raw file):
Previously, oncilla (Dominik Roos) wrote…
in that case the json tag should be
sequenceIMO
Done.
scion/cmd/scion/common.go line 57 at r6 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Why do we no longer return an error in case of unsupported format?
AFAICT, you always need to do the nil check now and the API of this function just became a little less obvious than with an explicit error return value.
Your previous comment mentioned removing the printf variable and return nil instead of an error. We still return an error if the function returns nil just on the caller side.
scion/cmd/scion/ping.go line 56 at r6 (raw file):
ping.Stats MinRTT time.Duration `json:"min_RTT" yaml:"min_RTT"` AvgRTT time.Duration `json:"avg_RTT" yaml:"avg_RTT"`
Done.
scion/cmd/scion/ping.go line 363 at r6 (raw file):
Previously, oncilla (Dominik Roos) wrote…
(also, for the record. I think making this a method with side effects is surprising.)
Done.
scion/cmd/scion/ping.go line 386 at r6 (raw file):
Previously, oncilla (Dominik Roos) wrote…
ditto
This function only computes the rtts values. So I think this could remain unchanged with the changes to pingSummary and moving the print RTT stuff to the switch statement.
scion/cmd/scion/traceroute.go line 52 at r5 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Yes, but why do you want these extra fields on the traceroute path?
From my point of view, either both ping and traceroute have them, or none.
The two commands are very similar in their purpose, so I would also expect the paths to contain the exact same amount of infromation.I think @matzf had in mind that you will adapt both and not just ping.
My bad then, removed it.
There was a problem hiding this comment.
Reviewable status: 11 of 15 files reviewed, 6 unresolved discussions (waiting on @oncilla)
scion/cmd/scion/ping.go line 363 at r6 (raw file):
Previously, bunert wrote…
Done.
Almost. This should not write to the ping.Stats struct but return a Stats object. I think that's what @oncilla meant also.
Perhaps it makes more sense to move Loss and Time to Stats too (currently in ping.Stats). Then it's clearer that all these fields should be populated here, in calculateStats.
Instead of
calculateStats(&stats, time.Since(start))
res.Statistics.Stats = stats
processRTTs(&res.Statistics, res.Replies)we should have
res.Statistics = calculateStats(stats, time.Since(start), res.Replies)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf and @oncilla)
scion/cmd/scion/common.go line 57 at r6 (raw file):
Previously, oncilla (Dominik Roos) wrote…
No, my previous comment was that you should return nil instead of an actual value for the first return value in case that the second return value (error) is non-nil.
Check out the suggestion I added (which you can copy paste one-to-one)
Got it thanks.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @bunert and @matzf)
scion/cmd/scion/common.go line 49 at r10 (raw file):
// getPrintf returns a printf function for the "human" formatting flag and an empty one for machine // readable format flags func getPrintf(outputFlag string, writer io.Writer) (
We do not split up the return values. So this would be formatted as
func getPrintf(
outputFlag string,
writer io.Writer,
) (func(format string, ctx ...interface{}), error) {
But we can just rename outputFlag to output and it will fit on one line.
There was a problem hiding this comment.
Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @matzf and @oncilla)
scion/cmd/scion/common.go line 49 at r10 (raw file):
Previously, oncilla (Dominik Roos) wrote…
We do not split up the return values. So this would be formatted as
func getPrintf( outputFlag string, writer io.Writer, ) (func(format string, ctx ...interface{}), error) {But we can just rename
outputFlagtooutputand it will fit on one line.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r11.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @matzf)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)
There was a problem hiding this comment.
Reviewed 1 of 11 files at r3, 7 of 13 files at r4, 1 of 7 files at r6, 1 of 2 files at r8, 1 of 3 files at r9, 3 of 4 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @bunert)
private/path/pathpol/sequence.go line 325 at r11 (raw file):
} hops = append(hops, hop(ifaces[len(ifaces)-1].IA, ifaces[len(ifaces)-1].ID, 0)) desc = strings.Join(hops, " ") + " "
Nit. This adds a trailing whitespace:
"sequence": "1-ff00:0:112#0,1 1-ff00:0:110#2,1 1-ff00:0:111#41,0 ",
^
Not sure if this is relevant for the operation of the sequence matcher, but in the output it looks a bit odd.
scion/cmd/scion/common.go line 1 at r11 (raw file):
package main
Missing license text.
scion/cmd/scion/common.go line 33 at r11 (raw file):
} // GetHops constructs a list of snet path interfaces from an snet path
nit, getHops.
scion/cmd/scion/ping.go line 60 at r11 (raw file):
AvgRTT time.Duration `json:"avg_rtt" yaml:"avg_rtt"` MaxRTT time.Duration `json:"max_rtt" yaml:"max_rtt"` MdevRTT time.Duration `json:"mdev_rtt" yaml:"mdev_rtt"`
Nit. I've noticed that in json, time.Duration is formatted as an integer number in nanoseconds, but in yaml it's a string including the unit. Ideally this would be consistent. IMO the number is more practical for consumers. Nanoseconds does not feel like a very natural unit for this, perhaps it could be expressed as a float in milliseconds instead.
If you want to tackle this, add a wrapper type that implements the yaml and json marshaller interfaces.
scion/cmd/scion/showpaths.go line 164 at r11 (raw file):
cmd.Flags().StringVar(&flags.tracer, "tracing.agent", "", "Tracing agent address") cmd.Flags().BoolVar(&flags.cfg.Epic, "epic", false, "Enable EPIC.") err := cmd.Flags().MarkDeprecated("json", "json flag is deprecated, use format flag")
Would be nice to still honor the --json flag even if it is deprecated. Something like this, somewhere before getPrintf :
if flags.json && !cmd.Flags().Lookup("format").Changed {
flags.format = "json"
}
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @matzf)
private/path/pathpol/sequence.go line 325 at r11 (raw file):
Previously, matzf (Matthias Frei) wrote…
Nit. This adds a trailing whitespace:
"sequence": "1-ff00:0:112#0,1 1-ff00:0:110#2,1 1-ff00:0:111#41,0 ", ^Not sure if this is relevant for the operation of the sequence matcher, but in the output it looks a bit odd.
Was caused due to the required trailing whitespace in the pathpol package. It is now added outside of the GetSequence function.
scion/cmd/scion/common.go line 1 at r11 (raw file):
Previously, matzf (Matthias Frei) wrote…
Missing license text.
Done.
scion/cmd/scion/ping.go line 60 at r11 (raw file):
Previously, matzf (Matthias Frei) wrote…
Nit. I've noticed that in json,
time.Durationis formatted as an integer number in nanoseconds, but in yaml it's a string including the unit. Ideally this would be consistent. IMO the number is more practical for consumers. Nanoseconds does not feel like a very natural unit for this, perhaps it could be expressed as a float in milliseconds instead.
If you want to tackle this, add a wrapper type that implements the yaml and json marshaller interfaces.
Yeah, but I was thinking that adding the unit does not really help when processing it in scripts and without unit, the default behavior seemed reasonable. Otherwise, we display milliseconds assuming the consumer is sure it is about milliseconds. What do you think?
scion/cmd/scion/showpaths.go line 164 at r11 (raw file):
Previously, matzf (Matthias Frei) wrote…
Would be nice to still honor the
--jsonflag even if it is deprecated. Something like this, somewhere beforegetPrintf:if flags.json && !cmd.Flags().Lookup("format").Changed { flags.format = "json" }
Done.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @bunert)
scion/cmd/scion/ping.go line 60 at r11 (raw file):
Previously, bunert (Tobias Buner) wrote…
Yeah, but I was thinking that adding the unit does not really help when processing it in scripts and without unit, the default behavior seemed reasonable. Otherwise, we display milliseconds assuming the consumer is sure it is about milliseconds. What do you think?
I agree, it's more convenient for processing to have a number value, not a string including a unit. Currently, the yaml output is formatted as a string with a unit -- in my comment I meant to propose changing the yaml output format to also only show a number.
I'm not sure I understand what you mean by "Otherwise, we display milliseconds assuming the consumer is sure it is about milliseconds"; this is true regardless of which unit the value is displayed in. IMO milliseconds would just be the most natural option.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)
scion/cmd/scion/ping.go line 60 at r11 (raw file):
Previously, matzf (Matthias Frei) wrote…
I agree, it's more convenient for processing to have a number value, not a string including a unit. Currently, the yaml output is formatted as a string with a unit -- in my comment I meant to propose changing the yaml output format to also only show a number.
I'm not sure I understand what you mean by "Otherwise, we display milliseconds assuming the consumer is sure it is about milliseconds"; this is true regardless of which unit the value is displayed in. IMO milliseconds would just be the most natural option.
Got it. I adjusted it to be milliseconds with 3 digits precision. To be consistent I also adjusted the traceroute RTTs.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r12, 1 of 2 files at r13, all commit messages.
Reviewable status: 14 of 15 files reviewed, 1 unresolved discussion (waiting on @matzf)
There was a problem hiding this comment.
Reviewable status: 14 of 15 files reviewed, 3 unresolved discussions (waiting on @bunert and @oncilla)
scion/cmd/scion/ping.go line 60 at r11 (raw file):
Previously, bunert (Tobias Buner) wrote…
Got it. I adjusted it to be milliseconds with 3 digits precision. To be consistent I also adjusted the traceroute RTTs.
Ok great, functionally that's exactly what I had in mind.
In terms of the implementation, I think it's not entirely ideal that this formatting logic is now replicated for every value.
One way to get around this would be to use a wrapper type for time.Duration to take care of the formatting. Here's a specific proposal (only for ping, traceroute not adapted). Feel free to apply this or change it as you see fit.
diff --git a/scion/cmd/scion/common.go b/scion/cmd/scion/common.go
index 89246babb..1bd4abe35 100644
--- a/scion/cmd/scion/common.go
+++ b/scion/cmd/scion/common.go
@@ -15,9 +15,12 @@
package main
import (
+ "encoding/json"
"fmt"
"io"
+ "math"
"net"
+ "time"
"github.com/scionproto/scion/pkg/addr"
"github.com/scionproto/scion/pkg/private/common"
@@ -72,3 +75,28 @@ func getPrintf(output string, writer io.Writer) (func(format string, ctx ...inte
return nil, serrors.New("format not supported", "format", output)
}
}
+
+type durationMillis time.Duration
+
+func (d durationMillis) String() string {
+ return fmt.Sprintf("%.3fms", d.Millis())
+}
+
+func (d durationMillis) MarshalJSON() ([]byte, error) {
+ return json.Marshal(d.MillisRounded())
+}
+
+func (d durationMillis) MarshalYAML() (interface{}, error) {
+ return d.MillisRounded(), nil
+}
+
+// millis returns the duration as a floating point number of milliseconds
+func (d durationMillis) Millis() float64 {
+ return float64(d) / 1e6
+}
+
+// millisRounded returns the duration as a floating point number of
+// milliseconds, rounded to microseconds (3 digits precision).
+func (d durationMillis) MillisRounded() float64 {
+ return math.Round(float64(d)/1000) / 1000
+}
diff --git a/scion/cmd/scion/ping.go b/scion/cmd/scion/ping.go
index 63289473f..36c8b2467 100644
--- a/scion/cmd/scion/ping.go
+++ b/scion/cmd/scion/ping.go
@@ -52,20 +52,20 @@ type Result struct {
type Stats struct {
ping.Stats
- Loss int `json:"packet_loss" yaml:"packet_loss"`
- Time float64 `json:"time" yaml:"time"`
- MinRTT float64 `json:"min_rtt" yaml:"min_rtt"`
- AvgRTT float64 `json:"avg_rtt" yaml:"avg_rtt"`
- MaxRTT float64 `json:"max_rtt" yaml:"max_rtt"`
- MdevRTT float64 `json:"mdev_rtt" yaml:"mdev_rtt"`
+ Loss int `json:"packet_loss" yaml:"packet_loss"`
+ Time durationMillis `json:"time" yaml:"time"`
+ MinRTT durationMillis `json:"min_rtt" yaml:"min_rtt"`
+ AvgRTT durationMillis `json:"avg_rtt" yaml:"avg_rtt"`
+ MaxRTT durationMillis `json:"max_rtt" yaml:"max_rtt"`
+ MdevRTT durationMillis `json:"mdev_rtt" yaml:"mdev_rtt"`
}
type PingUpdate struct {
- Size int `json:"scion_packet_size" yaml:"scion_packet_size"`
- Source string `json:"source_isd_as" yaml:"source_isd_as"`
- Sequence int `json:"scmp_seq" yaml:"scmp_seq"`
- RTT float64 `json:"round_trip_time" yaml:"round_trip_time"`
- State string `json:"state" yaml:"state"`
+ Size int `json:"scion_packet_size" yaml:"scion_packet_size"`
+ Source string `json:"source_isd_as" yaml:"source_isd_as"`
+ Sequence int `json:"scmp_seq" yaml:"scmp_seq"`
+ RTT durationMillis `json:"round_trip_time" yaml:"round_trip_time"`
+ State string `json:"state" yaml:"state"`
}
func newPing(pather CommandPather) *cobra.Command {
@@ -289,12 +289,12 @@ On other errors, ping will exit with code 2.
Size: update.Size,
Source: update.Source.String(),
Sequence: update.Sequence,
- RTT: float64(update.RTT.Nanoseconds()) / 1e6,
+ RTT: durationMillis(update.RTT),
State: update.State.String(),
})
printf("%d bytes from %s,%s: scmp_seq=%d time=%s%s\n",
update.Size, update.Source.IA, update.Source.Host, update.Sequence,
- fmt.Sprintf("%.3fms", float64(update.RTT.Nanoseconds())/1e6), additional)
+ durationMillis(update.RTT), additional)
},
})
if err != nil {
@@ -308,14 +308,14 @@ On other errors, ping will exit with code 2.
printf("\n--- %s,%s statistics ---\n", remote.IA, remote.Host.IP)
printf("%d packets transmitted, %d received, %d%% packet loss, time %v\n",
s.Sent, s.Received, res.Statistics.Loss,
- time.Duration(res.Statistics.Time)*time.Millisecond,
+ res.Statistics.Time,
)
if s.Received != 0 {
printf("rtt min/avg/max/mdev = %.3f/%.3f/%.3f/%.3f ms\n",
- res.Statistics.MinRTT,
- res.Statistics.AvgRTT,
- res.Statistics.MaxRTT,
- res.Statistics.MdevRTT,
+ res.Statistics.MinRTT.Millis(),
+ res.Statistics.AvgRTT.Millis(),
+ res.Statistics.MaxRTT.Millis(),
+ res.Statistics.MdevRTT.Millis(),
)
}
if stats.Received == 0 {
@@ -384,32 +384,35 @@ func calculateStats(s ping.Stats, replies []PingUpdate, run time.Duration) Stats
stats := Stats{
Stats: s,
Loss: loss,
- Time: math.Round((float64(run.Nanoseconds())/1e6)*1000) / 1000,
+ Time: durationMillis(run),
}
if len(replies) == 0 {
return stats
}
- stats.MinRTT = replies[0].RTT
- stats.MaxRTT = replies[0].RTT
- var sum float64
+ minRTT := replies[0].RTT
+ maxRTT := replies[0].RTT
+ var sum durationMillis
for i := 0; i < len(replies); i++ {
- if replies[i].RTT < stats.MinRTT {
- stats.MinRTT = replies[i].RTT
+ if replies[i].RTT < minRTT {
+ minRTT = replies[i].RTT
}
- if replies[i].RTT > stats.MaxRTT {
- stats.MaxRTT = replies[i].RTT
+ if replies[i].RTT > maxRTT {
+ maxRTT = replies[i].RTT
}
sum += replies[i].RTT
}
- stats.AvgRTT = math.Round(sum/float64(len(replies))*1000) / 1000
+ avgRTT := durationMillis(int(sum) / len(replies))
// standard deviation
var sd float64
for i := 0; i < len(replies); i++ {
- sd += math.Pow(replies[i].RTT-stats.AvgRTT, 2)
+ sd += math.Pow(float64(replies[i].RTT-avgRTT), 2)
}
- stats.MdevRTT = math.Round(math.Sqrt(sd/float64(len(replies)))*1000) / 1000
-
+ mdevRTT := math.Sqrt(sd / float64(len(replies)))
+ stats.MinRTT = durationMillis(minRTT)
+ stats.MaxRTT = durationMillis(maxRTT)
+ stats.AvgRTT = durationMillis(avgRTT)
+ stats.MdevRTT = durationMillis(mdevRTT)
return stats
}scion/cmd/scion/ping.go line 54 at r13 (raw file):
type Stats struct { ping.Stats
In yaml, this creates a stats sub-dictionary. Add inline annotation.
Suggestion:
ping.Stats `yaml:",inline"`scion/cmd/scion/ping.go line 65 at r13 (raw file):
type PingUpdate struct { Size int `json:"scion_packet_size" yaml:"scion_packet_size"` Source string `json:"source_isd_as" yaml:"source_isd_as"`
json/yaml tags don't match usage, the field contains the full SCION address not only ISD-AS.
There was a problem hiding this comment.
Reviewable status: 14 of 15 files reviewed, 3 unresolved discussions (waiting on @bunert and @matzf)
scion/cmd/scion/ping.go line 60 at r11 (raw file):
Previously, matzf (Matthias Frei) wrote…
Ok great, functionally that's exactly what I had in mind.
In terms of the implementation, I think it's not entirely ideal that this formatting logic is now replicated for every value.One way to get around this would be to use a wrapper type for
time.Durationto take care of the formatting. Here's a specific proposal (only for ping, traceroute not adapted). Feel free to apply this or change it as you see fit.diff --git a/scion/cmd/scion/common.go b/scion/cmd/scion/common.go index 89246babb..1bd4abe35 100644 --- a/scion/cmd/scion/common.go +++ b/scion/cmd/scion/common.go @@ -15,9 +15,12 @@ package main import ( + "encoding/json" "fmt" "io" + "math" "net" + "time" "github.com/scionproto/scion/pkg/addr" "github.com/scionproto/scion/pkg/private/common" @@ -72,3 +75,28 @@ func getPrintf(output string, writer io.Writer) (func(format string, ctx ...inte return nil, serrors.New("format not supported", "format", output) } } + +type durationMillis time.Duration + +func (d durationMillis) String() string { + return fmt.Sprintf("%.3fms", d.Millis()) +} + +func (d durationMillis) MarshalJSON() ([]byte, error) { + return json.Marshal(d.MillisRounded()) +} + +func (d durationMillis) MarshalYAML() (interface{}, error) { + return d.MillisRounded(), nil +} + +// millis returns the duration as a floating point number of milliseconds +func (d durationMillis) Millis() float64 { + return float64(d) / 1e6 +} + +// millisRounded returns the duration as a floating point number of +// milliseconds, rounded to microseconds (3 digits precision). +func (d durationMillis) MillisRounded() float64 { + return math.Round(float64(d)/1000) / 1000 +} diff --git a/scion/cmd/scion/ping.go b/scion/cmd/scion/ping.go index 63289473f..36c8b2467 100644 --- a/scion/cmd/scion/ping.go +++ b/scion/cmd/scion/ping.go @@ -52,20 +52,20 @@ type Result struct { type Stats struct { ping.Stats - Loss int `json:"packet_loss" yaml:"packet_loss"` - Time float64 `json:"time" yaml:"time"` - MinRTT float64 `json:"min_rtt" yaml:"min_rtt"` - AvgRTT float64 `json:"avg_rtt" yaml:"avg_rtt"` - MaxRTT float64 `json:"max_rtt" yaml:"max_rtt"` - MdevRTT float64 `json:"mdev_rtt" yaml:"mdev_rtt"` + Loss int `json:"packet_loss" yaml:"packet_loss"` + Time durationMillis `json:"time" yaml:"time"` + MinRTT durationMillis `json:"min_rtt" yaml:"min_rtt"` + AvgRTT durationMillis `json:"avg_rtt" yaml:"avg_rtt"` + MaxRTT durationMillis `json:"max_rtt" yaml:"max_rtt"` + MdevRTT durationMillis `json:"mdev_rtt" yaml:"mdev_rtt"` } type PingUpdate struct { - Size int `json:"scion_packet_size" yaml:"scion_packet_size"` - Source string `json:"source_isd_as" yaml:"source_isd_as"` - Sequence int `json:"scmp_seq" yaml:"scmp_seq"` - RTT float64 `json:"round_trip_time" yaml:"round_trip_time"` - State string `json:"state" yaml:"state"` + Size int `json:"scion_packet_size" yaml:"scion_packet_size"` + Source string `json:"source_isd_as" yaml:"source_isd_as"` + Sequence int `json:"scmp_seq" yaml:"scmp_seq"` + RTT durationMillis `json:"round_trip_time" yaml:"round_trip_time"` + State string `json:"state" yaml:"state"` } func newPing(pather CommandPather) *cobra.Command { @@ -289,12 +289,12 @@ On other errors, ping will exit with code 2. Size: update.Size, Source: update.Source.String(), Sequence: update.Sequence, - RTT: float64(update.RTT.Nanoseconds()) / 1e6, + RTT: durationMillis(update.RTT), State: update.State.String(), }) printf("%d bytes from %s,%s: scmp_seq=%d time=%s%s\n", update.Size, update.Source.IA, update.Source.Host, update.Sequence, - fmt.Sprintf("%.3fms", float64(update.RTT.Nanoseconds())/1e6), additional) + durationMillis(update.RTT), additional) }, }) if err != nil { @@ -308,14 +308,14 @@ On other errors, ping will exit with code 2. printf("\n--- %s,%s statistics ---\n", remote.IA, remote.Host.IP) printf("%d packets transmitted, %d received, %d%% packet loss, time %v\n", s.Sent, s.Received, res.Statistics.Loss, - time.Duration(res.Statistics.Time)*time.Millisecond, + res.Statistics.Time, ) if s.Received != 0 { printf("rtt min/avg/max/mdev = %.3f/%.3f/%.3f/%.3f ms\n", - res.Statistics.MinRTT, - res.Statistics.AvgRTT, - res.Statistics.MaxRTT, - res.Statistics.MdevRTT, + res.Statistics.MinRTT.Millis(), + res.Statistics.AvgRTT.Millis(), + res.Statistics.MaxRTT.Millis(), + res.Statistics.MdevRTT.Millis(), ) } if stats.Received == 0 { @@ -384,32 +384,35 @@ func calculateStats(s ping.Stats, replies []PingUpdate, run time.Duration) Stats stats := Stats{ Stats: s, Loss: loss, - Time: math.Round((float64(run.Nanoseconds())/1e6)*1000) / 1000, + Time: durationMillis(run), } if len(replies) == 0 { return stats } - stats.MinRTT = replies[0].RTT - stats.MaxRTT = replies[0].RTT - var sum float64 + minRTT := replies[0].RTT + maxRTT := replies[0].RTT + var sum durationMillis for i := 0; i < len(replies); i++ { - if replies[i].RTT < stats.MinRTT { - stats.MinRTT = replies[i].RTT + if replies[i].RTT < minRTT { + minRTT = replies[i].RTT } - if replies[i].RTT > stats.MaxRTT { - stats.MaxRTT = replies[i].RTT + if replies[i].RTT > maxRTT { + maxRTT = replies[i].RTT } sum += replies[i].RTT } - stats.AvgRTT = math.Round(sum/float64(len(replies))*1000) / 1000 + avgRTT := durationMillis(int(sum) / len(replies)) // standard deviation var sd float64 for i := 0; i < len(replies); i++ { - sd += math.Pow(replies[i].RTT-stats.AvgRTT, 2) + sd += math.Pow(float64(replies[i].RTT-avgRTT), 2) } - stats.MdevRTT = math.Round(math.Sqrt(sd/float64(len(replies)))*1000) / 1000 - + mdevRTT := math.Sqrt(sd / float64(len(replies))) + stats.MinRTT = durationMillis(minRTT) + stats.MaxRTT = durationMillis(maxRTT) + stats.AvgRTT = durationMillis(avgRTT) + stats.MdevRTT = durationMillis(mdevRTT) return stats }
I think if you use https://pkg.go.dev/encoding#TextMarshaler you can cover both JSON and YAML marshaling with one method.
There was a problem hiding this comment.
Reviewable status: 14 of 15 files reviewed, 3 unresolved discussions (waiting on @bunert and @oncilla)
scion/cmd/scion/ping.go line 60 at r11 (raw file):
The TextMarshaler works if we want to encode something as a JSON/YAML string, but we want it to be a number type.
In json:
Marshal calls its MarshalText method and encodes the result as a JSON string
The behaviour seems to be the same in yaml (but not documented).
There was a problem hiding this comment.
Reviewable status: 14 of 15 files reviewed, 3 unresolved discussions (waiting on @bunert and @matzf)
scion/cmd/scion/ping.go line 60 at r11 (raw file):
Previously, matzf (Matthias Frei) wrote…
The TextMarshaler works if we want to encode something as a JSON/YAML string, but we want it to be a number type.
In json:
Marshal calls its MarshalText method and encodes the result as a JSON string
The behaviour seems to be the same in yaml (but not documented).
Right, sorry for the confusion.
There was a problem hiding this comment.
Reviewable status: 12 of 15 files reviewed, 3 unresolved discussions (waiting on @matzf and @oncilla)
scion/cmd/scion/ping.go line 60 at r11 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Right, sorry for the confusion.
Great thanks a lot! Adjusted it with your patch and the traceroute part.
scion/cmd/scion/ping.go line 54 at r13 (raw file):
Previously, matzf (Matthias Frei) wrote…
In yaml, this creates a
statssub-dictionary. Add inline annotation.
Done.
scion/cmd/scion/ping.go line 65 at r13 (raw file):
Previously, matzf (Matthias Frei) wrote…
json/yaml tags don't match usage, the field contains the full SCION address not only ISD-AS.
Done.
There was a problem hiding this comment.
Reviewed 2 of 3 files at r14, all commit messages.
Reviewable status: 14 of 15 files reviewed, 1 unresolved discussion (waiting on @bunert and @oncilla)
scion/cmd/scion/traceroute.go line 214 at r14 (raw file):
res.Hops = append(res.Hops, HopInfo{ InterfaceID: uint16(update.Interface), IP: update.Remote.Host.IP().String(),
Sorry, just one more thing: this panics if Remote is nil. This occurs for hops where no reply packets are received. You can reproduce this by setting a very low timeout like 1us.
Side note: the traceroute implementation does not handle timeouts very gracefully. Replies arriving after the timeout are still processed and can even lead to negative RTTs for the subsequent packets. This could be fixed by setting the sequence number in the request packet and only process the responses with matching sequence IDs. I'll create an issue for this.
There was a problem hiding this comment.
Reviewable status: 14 of 15 files reviewed, 1 unresolved discussion (waiting on @matzf and @oncilla)
scion/cmd/scion/traceroute.go line 214 at r14 (raw file):
Previously, matzf (Matthias Frei) wrote…
Sorry, just one more thing: this panics if Remote is nil. This occurs for hops where no reply packets are received. You can reproduce this by setting a very low timeout like
1us.Side note: the traceroute implementation does not handle timeouts very gracefully. Replies arriving after the timeout are still processed and can even lead to negative RTTs for the subsequent packets. This could be fixed by setting the sequence number in the request packet and only process the responses with matching sequence IDs. I'll create an issue for this.
Should now check correctly if the remote is empty and act accordingly. The formatted output contains the interface ID and IA from the path resulting in the following example hop:
{
"interface_id": 41,
"ip": "",
"isd_as": "1-ff00:0:111",
"round_trip_times": null
}
There was a problem hiding this comment.
Reviewed 2 of 3 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)
There was a problem hiding this comment.
Thanks for your patience throughout the many feedback rounds. Great work!
Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @bunert)
scion/cmd/scion/traceroute.go line 214 at r14 (raw file):
Previously, bunert (Tobias Buner) wrote…
Should now check correctly if the remote is empty and act accordingly. The formatted output contains the interface ID and IA from the path resulting in the following example hop:
{ "interface_id": 41, "ip": "", "isd_as": "1-ff00:0:111", "round_trip_times": null }
💯
There was a problem hiding this comment.
Perhaps the generated command docs be updated as well now. Run bazel run //:update_all.
Reviewable status:
There was a problem hiding this comment.
Reviewed 3 of 3 files at r16, all commit messages.
Reviewable status:
Machine-readable output for the
sciontools (scion showpaths,scion ping,scion traceroute) offers the possibility to use them in scripts for easy processing. The deprecatedscion showpaths--jsonflag is replaced by the new--formatflag.This change is