add time in output lines #225

Ilhan-Personal · 2024-08-02T18:55:26Z

Important

Please provide the requested parameters below to help us review your pull request.

Pull requests without a body or explanation will be rejected.

Describe your changes

Issue ticket number and link

github.com//issues/215

Checklist before requesting a review

I have performed a self-review of my code
If it is a core feature, I have added tests.
I have run make check and there are no failures.

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

.gitignore

Ilhan-Personal · 2024-08-02T19:02:29Z

Output with the new changes looks something like this. @pouriyajamshidi could you please review? I also have a doubt, why does CodeFactor say I have issues in tcping.go when I did not make changes in that file? Thanks in advance :)

ilhan@syeds-MacBook-Pro tcping % go run . www.example.com 80 
2024-08-03 00:31:17 TCPinging www.example.com on port 80
2024-08-03 00:31:18 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=1 time=303.326 ms
2024-08-03 00:31:19 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=2 time=263.166 ms
2024-08-03 00:31:20 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=3 time=350.889 ms
2024-08-03 00:31:21 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=4 time=272.626 ms
2024-08-03 00:31:22 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=5 time=296.352 ms
2024-08-03 00:31:23 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=6 time=256.992 ms
2024-08-03 00:31:24 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=7 time=300.664 ms
2024-08-03 00:31:24 No reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=1
2024-08-03 00:31:25 No reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=2
2024-08-03 00:31:26 No reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=3
2024-08-03 00:31:27 No reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=4
2024-08-03 00:31:28 No reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=5
2024-08-03 00:31:29 No reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=6
2024-08-03 00:31:31 No response received for 6 seconds
2024-08-03 00:31:31 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=1 time=929.736 ms
2024-08-03 00:31:32 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=2 time=451.907 ms
2024-08-03 00:31:33 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=3 time=480.420 ms
2024-08-03 00:31:34 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=4 time=377.191 ms

2024-08-03 00:31:35 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=5 time=298.571 ms

--- www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) TCPing statistics ---
18 probes transmitted on port 80 | 12 received, 33.33% packet loss
successful probes:   12
unsuccessful probes: 6
last successful probe:   2024-08-03 00:31:34
last unsuccessful probe: 2024-08-03 00:31:29
total uptime:   12 seconds
total downtime: 6 seconds
longest consecutive uptime:   7 seconds from 2024-08-03 00:31:17 to 2024-08-03 00:31:24
longest consecutive downtime: 6 seconds from 2024-08-03 00:31:24 to 2024-08-03 00:31:30
retried to resolve hostname 0 times
rtt min/avg/max: 256.992/381.820/929.736 ms
--------------------------------------
TCPing started at: 2024-08-03 00:31:17
duration (HH:MM:SS): 00:00:18

2024-08-03 00:31:36 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=6 time=240.240 ms
2024-08-03 00:31:37 Reply from www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) on port 80 TCP_conn=7 time=250.638 ms
^C
--- www.example.com (2606:2800:21f:cb07:6820:80da:af6b:8b2c) TCPing statistics ---
20 probes transmitted on port 80 | 14 received, 30.00% packet loss
successful probes:   14
unsuccessful probes: 6
last successful probe:   2024-08-03 00:31:36
last unsuccessful probe: 2024-08-03 00:31:29
total uptime:   14 seconds
total downtime: 6 seconds
longest consecutive uptime:   7 seconds from 2024-08-03 00:31:17 to 2024-08-03 00:31:24
longest consecutive downtime: 6 seconds from 2024-08-03 00:31:24 to 2024-08-03 00:31:30
retried to resolve hostname 0 times
rtt min/avg/max: 240.240/362.337/929.736 ms
--------------------------------------
TCPing started at: 2024-08-03 00:31:17
TCPing ended at:   2024-08-03 00:31:37
duration (HH:MM:SS): 00:00:20

pouriyajamshidi · 2024-08-02T19:18:33Z

Hi @SYSHIL,

I had a very quick look at your PR and the first thing that jumped at me is that the default behavior has changed.

This is not the intention.

We try to stay relatively close to what Linux's ping utility does and as the issue was describing, timestamps are only activated using the -D flag.

We also have other printers than the plane printer. This change has to cover what is implemented here:

tcping/tcping.go

Line 230 in 2ae984d

    
           func setPrinter(tcping *tcping, outputJSON, prettyJSON *bool, outputDb *string, args []string) {

Let's address the rest of the open topics after that.

Ilhan-Personal · 2024-08-03T11:15:18Z

Understood, Thank you @pouriyajamshidi I'll look into this further

Ilhan-Personal · 2024-08-05T13:52:50Z

Hey @pouriyajamshidi I added a boolean field in the plane printer to ensure the default behaviour doesn't get affected and the timestamp is only added to our logs when a -D flag is passed.
here is the sample usage, could you please check? in case its okay i can update our READMEs too

2024-08-05 19:21:49 TCPinging www.google.com on port 80
2024-08-05 19:21:49 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=1
2024-08-05 19:21:50 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=2
2024-08-05 19:21:51 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=3
2024-08-05 19:21:52 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=4
2024-08-05 19:21:53 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=5
2024-08-05 19:21:54 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=6
2024-08-05 19:21:55 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=7
2024-08-05 19:21:56 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=8
2024-08-05 19:21:57 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=9
2024-08-05 19:21:58 No reply from www.google.com (142.250.195.196) on port 80 TCP_conn=1
2024-08-05 19:21:59 No reply from www.google.com (142.250.195.196) on port 80 TCP_conn=2
2024-08-05 19:22:00 No reply from www.google.com (142.250.195.196) on port 80 TCP_conn=3
2024-08-05 19:22:01 No reply from www.google.com (142.250.195.196) on port 80 TCP_conn=4
2024-08-05 19:22:02 No reply from www.google.com (142.250.195.196) on port 80 TCP_conn=5
2024-08-05 19:22:03 No response received for 5 seconds
2024-08-05 19:22:03 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=1
2024-08-05 19:22:04 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=2
2024-08-05 19:22:05 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=3
2024-08-05 19:22:06 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=4
2024-08-05 19:22:07 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=5

pouriyajamshidi · 2024-08-05T15:14:46Z

Hey @pouriyajamshidi I added a boolean field in the plane printer to ensure the default behaviour doesn't get affected and the timestamp is only added to our logs when a -D flag is passed. here is the sample usage, could you please check? in case its okay i can update our READMEs too

2024-08-05 19:21:49 TCPinging www.google.com on port 80
2024-08-05 19:21:49 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=1
2024-08-05 19:21:50 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=2
2024-08-05 19:21:51 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=3
2024-08-05 19:21:52 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=4
2024-08-05 19:21:53 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=5
2024-08-05 19:21:54 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=6
2024-08-05 19:21:55 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=7
2024-08-05 19:21:56 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=8
2024-08-05 19:21:57 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=9
2024-08-05 19:21:58 No reply from www.google.com (142.250.195.196) on port 80 TCP_conn=1
2024-08-05 19:21:59 No reply from www.google.com (142.250.195.196) on port 80 TCP_conn=2
2024-08-05 19:22:00 No reply from www.google.com (142.250.195.196) on port 80 TCP_conn=3
2024-08-05 19:22:01 No reply from www.google.com (142.250.195.196) on port 80 TCP_conn=4
2024-08-05 19:22:02 No reply from www.google.com (142.250.195.196) on port 80 TCP_conn=5
2024-08-05 19:22:03 No response received for 5 seconds
2024-08-05 19:22:03 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=1
2024-08-05 19:22:04 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=2
2024-08-05 19:22:05 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=3
2024-08-05 19:22:06 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=4
2024-08-05 19:22:07 Reply from www.google.com (142.250.195.196) on port 80 TCP_conn=5

Hi @SYSHIL,

Sorry, I had to be more concise. tcping's printer comes in three forms:

plane printer (the one you have been working on)
json printer which is activated by the -j and -j --pretty flags
database printer which is activated by --db flag

When making changes to the way we display information, we need to ensure all of these printers outputting the same data to prevent any unexpected behavior for our users.

Now, what needs to be done is to implement the same for json and db printers.

Hopefully, you still have patience for this. 😄

Ilhan-Personal · 2024-08-05T20:22:15Z

Hey @pouriyajamshidi, Yeah the reason I did not touch our json printer and database printer was because they already seem to be printing our current timestamp. here and here so this -D flag would only extend the capability of the plane printer. Sorry if I'm misunderstanding this and thank you for bearing with me 🙏

pouriyajamshidi · 2024-08-05T20:38:02Z

Hey @pouriyajamshidi, Yeah the reason I did not touch our json printer and database printer was because they already seem to be printing our current timestamp. here and here so this -D flag would only extend the capability of the plane printer. Sorry if I'm misunderstanding this and thank you for bearing with me 🙏

ah, sorry

I will have a closer look tomorrow.

statsprinter.go

Ilhan-Personal · 2024-08-08T14:19:16Z

Hey @pouriyajamshidi could u please check now, Thank you :)

pouriyajamshidi · 2024-08-08T18:03:07Z

Hey @pouriyajamshidi could u please check now, Thank you :)

Sure. Will get to this in a day or two max.

pouriyajamshidi · 2024-08-08T19:59:32Z

Hey @SYSHIL,

I can see the reviews I left for not needing to timestamp certain messages are marked as solved but they are still using the PrintOptions struct with showTimestamp set to false.

I do not see any benefit in doing that. Please just revert them to what they were to keep the code simple.

Ilhan-Personal · 2024-08-09T15:11:52Z

Ahh sorry @pouriyajamshidi, somehow I missed that. Removed it now

statsprinter.go

pouriyajamshidi · 2024-08-09T17:58:33Z

Hey @SYSHIL

Could you please review your PR once while you are in a good head-space?

I think you have missed a couple of changes I have requested and every time I see "done or fixed", I take a look at the code and it confuses me more. 😄

Thanks

Ilhan-Personal · 2024-08-09T18:02:11Z

Sure @pouriyajamshidi I will review the entire PR and ping you 🙏

Ilhan-Personal · 2024-08-09T18:21:51Z

Alright time in output lines only shows up upon printprobefailure and printprobesuccess but I don't think I quite understood this comment. Could you please elaborate on this @pouriyajamshidi ?

I can see the reviews I left for not needing to timestamp certain messages are marked as solved but they are still using the PrintOptions struct with showTimestamp set to false.

They do use PrintOptions struct but the showTimestamp field is not set to false its set to nil by default because its a pointer to a boolean. I can see the need to not tweak the existing function at all and revert my changes but I thought it would be cleaner to just have one way of printing things through the plane printer in this case it is through the printReply function.

pouriyajamshidi · 2024-08-10T09:39:07Z

Alright time in output lines only shows up upon printprobefailure and printprobesuccess but I don't think I quite understood this comment. Could you please elaborate on this @pouriyajamshidi ?

I can see the reviews I left for not needing to timestamp certain messages are marked as solved but they are still using the PrintOptions struct with showTimestamp set to false.

They do use PrintOptions struct but the showTimestamp field is not set to false its set to nil by default because its a pointer to a boolean. I can see the need to not tweak the existing function at all and revert my changes but I thought it would be cleaner to just have one way of printing things through the plane printer in this case it is through the printReply function.

This is what I meant.

I don't see any point in using PrintOptions when you don't need it.

Ilhan-Personal · 2024-08-10T10:27:40Z

This is what I meant.
I don't see any point in using PrintOptions when you don't need it.

Okay @pouriyajamshidi my initial idea was to just have one way of printing things through the planPrinter and that's why I added printOptions in case there's a need to extend it but I also agree with keeping the code simple, I've reverted my changes in functions where printoptions isn't really helping us

statsprinter.go

tcping.go

statsprinter.go

pouriyajamshidi

Thanks for the contribution @SYSHIL

Upon making the final changes, this can get merged in

statsprinter.go

Ilhan-Personal · 2024-08-22T18:16:55Z

I've made the appropriate changes @pouriyajamshidi, Sorry about the initial confusion with respect to requirements....Would love to collaborate soon if there any new issues I could pick up :)

pouriyajamshidi · 2024-08-23T16:29:13Z

I've made the appropriate changes @pouriyajamshidi, Sorry about the initial confusion with respect to requirements....Would love to collaborate soon if there any new issues I could pick up :)

Thanks for pushing this through to the end.

Perhaps solving the CI error next would be a good move?

Nonetheless, appreciate the help

pouriyajamshidi

LGTM

closes #215

Ilhan-Personal and others added 4 commits August 3, 2024 00:22

add time in output lines

d4136d3

Delete .tool-versions

41f73e7

nits

e9f1ddf

eol

60a4245

Ilhan-Personal commented Aug 2, 2024

View reviewed changes

.gitignore Show resolved Hide resolved

pouriyajamshidi assigned Ilhan-Personal Aug 2, 2024

pouriyajamshidi added the enhancement New feature or request label Aug 2, 2024

fixes

d666321

Merge branch 'master' into ilu/add-timestamp

d22a6cf

pouriyajamshidi requested changes Aug 6, 2024

View reviewed changes

Ilhan-Personal added 2 commits August 7, 2024 23:02

fixes

cb4406b

fix

1d918a3

fix

0f9ecd5

pouriyajamshidi requested changes Aug 9, 2024

View reviewed changes

statsprinter.go Outdated Show resolved Hide resolved

fix color

e2711eb

revert changes to functions that dont require printOptions

d797386

Ilhan-Personal commented Aug 10, 2024

View reviewed changes

statsprinter.go Outdated Show resolved Hide resolved

pouriyajamshidi requested changes Aug 16, 2024

View reviewed changes

statsprinter.go Outdated Show resolved Hide resolved

tcping.go Show resolved Hide resolved

statsprinter.go Outdated Show resolved Hide resolved

Ilhan-Personal and others added 3 commits August 20, 2024 01:13

remove enums and reply function

c6b5f19

nit

6628929

Merge branch 'master' into ilu/add-timestamp

05873aa

pouriyajamshidi reviewed Aug 20, 2024

View reviewed changes

statsprinter.go Outdated Show resolved Hide resolved

pouriyajamshidi requested changes Aug 20, 2024

View reviewed changes

statsprinter.go Outdated Show resolved Hide resolved

statsprinter.go Outdated Show resolved Hide resolved

make code readable

10bddd5

pouriyajamshidi approved these changes Aug 23, 2024

View reviewed changes

pouriyajamshidi mentioned this pull request Aug 23, 2024

add time in output line please #215

Closed

pouriyajamshidi merged commit ea495b0 into pouriyajamshidi:master Aug 23, 2024
2 of 3 checks passed

Ilhan-Personal mentioned this pull request Sep 1, 2024

Fix CI issue #231

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add time in output lines #225

add time in output lines #225

Ilhan-Personal commented Aug 2, 2024 •

edited

Loading

Ilhan-Personal commented Aug 2, 2024 •

edited

Loading

pouriyajamshidi commented Aug 2, 2024 •

edited

Loading

Ilhan-Personal commented Aug 3, 2024

Ilhan-Personal commented Aug 5, 2024 •

edited

Loading

pouriyajamshidi commented Aug 5, 2024

Ilhan-Personal commented Aug 5, 2024

pouriyajamshidi commented Aug 5, 2024

Ilhan-Personal commented Aug 8, 2024

pouriyajamshidi commented Aug 8, 2024

pouriyajamshidi commented Aug 8, 2024

Ilhan-Personal commented Aug 9, 2024

pouriyajamshidi commented Aug 9, 2024

Ilhan-Personal commented Aug 9, 2024 •

edited

Loading

Ilhan-Personal commented Aug 9, 2024 •

edited

Loading

pouriyajamshidi commented Aug 10, 2024 •

edited

Loading

Ilhan-Personal commented Aug 10, 2024 •

edited

Loading

pouriyajamshidi left a comment

Ilhan-Personal commented Aug 22, 2024

pouriyajamshidi commented Aug 23, 2024

pouriyajamshidi left a comment •

edited

Loading

add time in output lines #225

add time in output lines #225

Conversation

Ilhan-Personal commented Aug 2, 2024 • edited Loading

Important

Describe your changes

Issue ticket number and link

Checklist before requesting a review

Type of change

Ilhan-Personal commented Aug 2, 2024 • edited Loading

pouriyajamshidi commented Aug 2, 2024 • edited Loading

Ilhan-Personal commented Aug 3, 2024

Ilhan-Personal commented Aug 5, 2024 • edited Loading

pouriyajamshidi commented Aug 5, 2024

Ilhan-Personal commented Aug 5, 2024

pouriyajamshidi commented Aug 5, 2024

Ilhan-Personal commented Aug 8, 2024

pouriyajamshidi commented Aug 8, 2024

pouriyajamshidi commented Aug 8, 2024

Ilhan-Personal commented Aug 9, 2024

pouriyajamshidi commented Aug 9, 2024

Ilhan-Personal commented Aug 9, 2024 • edited Loading

Ilhan-Personal commented Aug 9, 2024 • edited Loading

pouriyajamshidi commented Aug 10, 2024 • edited Loading

Ilhan-Personal commented Aug 10, 2024 • edited Loading

pouriyajamshidi left a comment

Choose a reason for hiding this comment

Ilhan-Personal commented Aug 22, 2024

pouriyajamshidi commented Aug 23, 2024

pouriyajamshidi left a comment • edited Loading

Choose a reason for hiding this comment

Ilhan-Personal commented Aug 2, 2024 •

edited

Loading

Ilhan-Personal commented Aug 2, 2024 •

edited

Loading

pouriyajamshidi commented Aug 2, 2024 •

edited

Loading

Ilhan-Personal commented Aug 5, 2024 •

edited

Loading

Ilhan-Personal commented Aug 9, 2024 •

edited

Loading

Ilhan-Personal commented Aug 9, 2024 •

edited

Loading

pouriyajamshidi commented Aug 10, 2024 •

edited

Loading

Ilhan-Personal commented Aug 10, 2024 •

edited

Loading

pouriyajamshidi left a comment •

edited

Loading