Test-Connection Changes by vexx32 · Pull Request #172 · PowerShell/PowerShell-RFC · GitHub

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

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

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!

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

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

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

So we'd need to put at least those methods into the code base for that to happen. Staffan Gustafsson (@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... Paul Higinbotham (@PaulHigin)?

Steve Lee (@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.

Update RFC per Steve's comments

Update MtuSize property type & give default value

This name conveys the same thing without being excessively long and causing difficulty effectively formatting the output.

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).

Steve Lee (@SteveL-MSFT) Joey Aiello (@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:

Using a Stopwatch to get reasonably accurate ping latency times.
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.

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

Rain Sallow (/u/ta11ow) (@vexx32) for the latency times, I think you can just send another ping to the destination along the route

Steve Lee (@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?

Update parameter sets

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

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.)

Update RFC & examples

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

This would be the default parameter set, correct?

Yep!

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

@PowerShell/powershell-committee voted and is ready to accept. James Truher (@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.

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

James Truher (@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?

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.

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

Joey Aiello (@joeyaiello) simple addition, I'm happy to. ^^

Rain Sallow (/u/ta11ow) (vexx32) added 2 commits May 9, 2019 10:59

Create RFCNNNN-Test-Connection.md

2633d63

Update RFCNNNN-Test-Connection.md

f2d2fbc

Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request May 9, 2019

Test-Connection cmdlet displaying unwanted data. PowerShell/PowerShell#6768

Closed

Steve Lee (SteveL-MSFT) reviewed May 10, 2019

View reviewed changes

Comment thread 1-Draft/RFCNNNN-Test-Connection.md Outdated

Rain Sallow (/u/ta11ow) (vexx32) added 2 commits May 24, 2019 18:40

Update RFCNNNN-Test-Connection.md

67d894f

Update RFC per Steve's comments

Update RFCNNNN-Test-Connection.md

771e964

Update MtuSize property type & give default value

Steve Lee (SteveL-MSFT) requested changes May 28, 2019

View reviewed changes

Comment thread 1-Draft/RFCNNNN-Test-Connection.md Outdated

📝 Rename RoundTripTime -> Latency

f6dc579

This name conveys the same thing without being excessively long and causing difficulty effectively formatting the output.

Steve Lee (SteveL-MSFT) approved these changes Jun 10, 2019

View reviewed changes

Joey Aiello (joeyaiello) added the Review - Committee label Jun 10, 2019

Rain Sallow (/u/ta11ow) (vexx32) commented Jun 20, 2019

View reviewed changes

Comment thread 1-Draft/RFCNNNN-Test-Connection.md Outdated

Comment thread 1-Draft/RFCNNNN-Test-Connection.md Outdated

Comment thread 1-Draft/RFCNNNN-Test-Connection.md Outdated

Comment thread 1-Draft/RFCNNNN-Test-Connection.md Outdated

Comment thread 1-Draft/RFCNNNN-Test-Connection.md Outdated

Apply suggestions from code review

6b92b9a

Update parameter sets

Update RFCNNNN-Test-Connection.md

deb5d3f

Update RFC & examples

Steve Lee (SteveL-MSFT) requested changes Jul 1, 2019

View reviewed changes

James Truher (JamesWTruher) reviewed Jul 1, 2019

View reviewed changes

Prepare for acceptance of RFC0037 (Test-Connection)

7a3af51

Joey Aiello (joeyaiello) merged commit b4fc60d into PowerShell:master Jul 1, 2019

Joey Aiello (joeyaiello) added Committee-Reviewed and removed Review - Committee labels Jul 1, 2019

Rain Sallow (/u/ta11ow) (vexx32) deleted the patch-1 branch July 1, 2019 19:18

Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Jul 1, 2019

Refactor Test-Connection PowerShell/PowerShell#10044

Closed

12 tasks

Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Oct 3, 2019

Test-Connection - Improve Logic and Output PowerShell/PowerShell#10697

Merged

14 tasks

+              }
+              // For -MtuSizeDetect switch
+              class PingMtuDetect : PingStatus

+              Test-Connection [-TargetName] <string[]> [-IPv4] [-IPv6] [-ResolveDestination] [-TimeoutSeconds <int>] [-Quiet] [<CommonParameters>]
+              # Set 1
+              [-Ping] [-Count <int>] [-Delay <int>] [-Source <string>] [-MaxHops <int>] [-BufferSize <int>] [-DontFragment]

Test-Connection Changes by vexx32 · Pull Request #172 · PowerShell/PowerShell-RFC · GitHub

Uh oh!

Test-Connection Changes - #172

Test-Connection Changes#172Joey Aiello (joeyaiello) merged 8 commits intoPowerShell:masterPowerShell/PowerShell-RFC:masterfrom vexx32:patch-1vexx32/PowerShell-RFC:patch-1Copy head branch name to clipboard

Conversation

Rain Sallow (/u/ta11ow) (vexx32) commented May 9, 2019

Uh oh!

Steve Lee (SteveL-MSFT) May 10, 2019

Choose a reason for hiding this comment

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) May 10, 2019

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) May 24, 2019

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) May 28, 2019

Choose a reason for hiding this comment

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Joey Aiello (joeyaiello) commented Jun 10, 2019

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Steve Lee (SteveL-MSFT) commented Jun 21, 2019

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Joey Aiello (joeyaiello) commented Jun 24, 2019

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Jun 27, 2019

Uh oh!

Steve Lee (SteveL-MSFT) Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

James Truher (JamesWTruher) left a comment

Choose a reason for hiding this comment

Uh oh!

Joey Aiello (joeyaiello) commented Jul 1, 2019

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobfrankly commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Joey Aiello (joeyaiello) commented Jul 2, 2019

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Test-Connection Changes#172
Joey Aiello (joeyaiello) merged 8 commits into
PowerShell:masterfrom
vexx32:patch-1

Steve Lee (SteveL-MSFT) May 24, 2019 •
edited
Loading

Rain Sallow (/u/ta11ow) (vexx32) May 28, 2019 •
edited
Loading

Rain Sallow (/u/ta11ow) (vexx32) commented Jun 19, 2019 •
edited
Loading

Rain Sallow (/u/ta11ow) (vexx32) commented Jun 21, 2019 •
edited
Loading

Rain Sallow (/u/ta11ow) (vexx32) commented Jun 24, 2019 •
edited
Loading

Rain Sallow (/u/ta11ow) (vexx32) commented Jul 1, 2019 •
edited
Loading

bobfrankly commented Jul 2, 2019 •
edited
Loading