Test-Connection Changes

[vexx32]

Refer to PowerShell/PowerShell#6768 for existing discussion and Committee decision details.

[SteveL-MSFT]

Did you consider just having MtuSize being a member of PingStatus but $null if -DetectMtuSize is not specified?

[vexx32]

We can do this, absolutely. However, that means that the default format view with the default behaviour will always have an empty column.

That strikes me as... Odd? Also, given the cmdlet's current behaviour with -MtuSizeDetect which causes the cmdlet to only send / retrieve a single PingReply (vs default behaviour W/O the switch being to send 4 pings instead) it seems a little better to have this as a separate object type imo.

Definitely open to alternate solutions though!

[SteveL-MSFT]

You've reminded me that @powercode has a PR to only show properties of MeasureInfo objects that have values. We can do the same thing here.

[vexx32]

Can you link me to that? I wasn't aware that the formatter supported this!

[SteveL-MSFT]

PowerShell/PowerShell#7104, this was merged but was reverted

[vexx32]

So we'd need to put at least those methods into the code base for that to happen. @powercode, would you be able to PR just those formatting methods in so that we can make use of them? I'm not familiar with why it was reverted, but I'm sure we could stand to use a feature like that in more places than just here. 🙂

EDIT: Seeing references to a security review. I guess that means we're waiting on a green light from... @PaulHigin?

[vexx32]

@SteveL-MSFT I looked over these again -- I know you opened an issue for it in the PS repo -- and I realize we're going to hit an issue here; the reason this is fairly easily doable for MeasureInfo is that the format used is a List format, not a Table format. Implementing the same thing in a table format is... problematic at best.

[joeyaiello]

I'm in general support barring the remainder of the discussion the formatting (which again, I think should be newlined for units and nothing else).

[vexx32]

@SteveL-MSFT @joeyaiello I was reading documentation for PingReply today... found this: https://docs.microsoft.com/en-us/dotnet/api/system.net.networkinformation.pingreply?view=netframework-4.8#remarks

Basically, when we're doing a traceroute (which essentially is done by sending out pings with TTL that increases one at a time so we can determine the hop points) we're always going to get back replies with a status of TtlExpired. This appears to be by design(?), and means that we're always going to have useless values for RoundtripTime, PingOptions, and Buffer.

I can work around this by:

1. Using a Stopwatch to get reasonably accurate ping latency times.
2. Explicitly passing in the PingOptions & buffer size values.

This shouldn't be hard to do, but I thought it should be noted. Not sure if it should be in the RFC text itself, let me know if I should add it.

This is why the current implementation can't get response times, etc., from the intermediary hops when doing a trace route -- so we have to basically fill in what we can by what we're sending as well as the directly measurable response time.

[vexx32]

Pointing out a couple things I missed / typo'd in case folks want to comment on the individual items before I incorporate the edits.

[SteveL-MSFT]

@vexx32 for the latency times, I think you can just send another ping to the destination along the route

[vexx32]

@SteveL-MSFT yeah, I considered that... but then we're potentially sending double (or more if the timer ping happens to fail) the number of pings every single time, and that... doesn't seem like a good idea, really?

If I stopwatch.Start(); reply = pinger.Ping(); stopwatch.Stop() it should be sufficiently accurate, no?

[joeyaiello]

No quorum today, but after applying your latest comments on missing a property or two, I'm personally good to merge.

[vexx32]

Sounds good. I'll do a full review of this today or tomorrow. I've been exploring this in code to make sure the RFC makes sense practically and I think there are one or two other things that may need to be updated as well. (Especially regarding my comment that the table formatter can't really handle the variable number of properties as originally suggested.)

[vexx32]

@joeyaiello @SteveL-MSFT Updated the RFC & included some new examples based on my POC implementation I've been working on. 😄

[SteveL-MSFT]

This would be the default parameter set, correct?

[vexx32]

Yep!

[JamesWTruher]

i'm not sure why -tcpport is its own parameter set and why it can't just be part of all of the sets.

[joeyaiello]

@PowerShell/powershell-committee voted and is ready to accept. @JamesWTruher had a question above, and I had one around whether -Source will actually work cross-platform (it's currently broken on Core), but we're comfortable handling both in code PR.

[vexx32]

Sounds good. I have this all implemented in a branch, so I'll send a PR this evening :)

@JamesWTruher -TcpPort current implementation is -Quiet by default, with no other option available. I'm not familiar enough with the low level APIs to know whether ping packets sent to other ports make sense in a traceroute context or with determining MTU size?

[bobfrankly]

I would like to see a sequence number added to the basic ping output. If you're testing on-prem targets that return consistent (like EXACTLY) stats, the sequence incrementing on the screen can be the difference between "is the ping still running? did the window freeze?" and "no, it's still running, see the sequence?"

Sequence numbers in the pings would also help with looking at the results from a long running ping test. When filtering the return from a long-running test, you could see if the missed pings are grouped or scattered by the sequence numbers.

[joeyaiello]

Unless @vexx32 wants to implement that immediately, I think it's fine to take that up as an issue/PR in PowerShell/PowerShell later.

[vexx32]

@joeyaiello simple addition, I'm happy to. ^^