Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools/tcpconnect: add option -c to count connects #2475

Merged
merged 1 commit into from Aug 1, 2019

Conversation

boat0
Copy link
Contributor

@boat0 boat0 commented Jul 30, 2019

Add -c to count all active connections per dest ip/port so we can
easily spot the heavy outbound connection attempts.

# ./tcpconnect.py -c
Tracing connect ... Hit Ctrl-C to end
^C
LADDR                 RADDR:RPORT             CONNECTS
192.168.10.50    <-> 172.217.21.194:443         70
192.168.10.50    <-> 172.213.11.195:443         34
192.168.10.50    <-> 172.212.22.194:443         21
[...]

@yonghong-song
Copy link
Collaborator

[buildbot, test this please]

@yonghong-song
Copy link
Collaborator

[buildbot, ok to test]

@@ -25,6 +25,9 @@ Print usage message.
\-t
Include a timestamp column.
.TP
\-c
Count connects per dest ip/port.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use "connections" here, which is more common usage, instead of "connects"? Same for below several other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"connects" may be more accurate than connections in this context? because we inspect the connect actions, not any already established connections.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use "new connections" or "connects". I guess "connect" okay which is short and also captures the essence.

.TP
Trace UID 1000 only:
#
.B tcpconnect \-u 1000
.TP
Count connects per dest ip/port:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be source ip + dest ip/port?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. will fix it

"""
struct ipv6_flow_key_t flow_key = {};
flow_key.saddr = skp->__sk_common.skc_rcv_saddr;
flow_key.daddr = skp->__sk_common.skc_daddr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above to calculate ipv6 src/dest addresses are not correct. Do you have environment to test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy and paste error. will fix it.

@boat0 boat0 force-pushed the tcpconnect branch 2 times, most recently from 3a758b8 to 27a7a9e Compare August 1, 2019 11:18
@yonghong-song
Copy link
Collaborator

Thanks for the fix. I have one more request for this.
On an ipv6 machine, I got:

-bash-4.4$ sudo ./tcpconnect.py -c
Tracing connect ... Hit Ctrl-C to end
^C
LADDR                     RADDR:RPORT               CONNECTS  
127.0.0.1            <-> 127.0.0.1:1456                5
127.0.0.1            <-> 127.0.0.1:2377                1
2401:db00:1120:405d:face:0:1d:0 <-> 2401:db00:1120:4190:face:0:3d:0:6969         22
2401:db00:1120:405d:face:0:1d:0 <-> 2401:db00:1120:6067:face:0:39:0:6969         18
2401:db00:1120:405d:face:0:1d:0 <-> 2401:db00:1120:405d:face:0:1d:0:4330          2
::1                  <-> ::1:16262                     2
...

You can see ipv4 and ipv6 addresses are printed differently.
ip4 address/port like 127.0.0.1:1456 looks okay with only one : as the separator.
ipv6 address/port separation is not really clear.
Could you print RADDR and RPORT separately just like plain tccconnect.py does?

@brendangregg
Copy link
Member

FWIW, I would not want to see this behavior added to all the tools. That the tools are (or were) simple and have a single function is a benefit that aids learning, and makes them double as code examples. I'd add switches like this only when necessary.

Add -c to count all active connections per dest ip/port so we can
easily spot the heavy outbound connection attempts.

    # ./tcpconnect.py -c
    Tracing connect ... Hit Ctrl-C to end
    ^C
    LADDR                 RADDR                    RPORT              CONNECTS
    192.168.10.50         172.217.21.194           443                70
    192.168.10.50         172.213.11.195           443                34
    192.168.10.50         172.212.22.194           443                21
    [...]
@boat0
Copy link
Contributor Author

boat0 commented Aug 1, 2019

FWIW, I would not want to see this behavior added to all the tools. That the tools are (or were) simple and have a single function is a benefit that aids learning, and makes them double as code examples. I'd add switches like this only when necessary.

Got your point. Plan is to integrate various BCC tools into production monitoring systems. So '-c' will make tcpconnect(maybe other tools as well) more efficient and more succinct. Personally I think this feature will be useful to others too.

@boat0
Copy link
Contributor Author

boat0 commented Aug 1, 2019

You can see ipv4 and ipv6 addresses are printed differently.
ip4 address/port like 127.0.0.1:1456 looks okay with only one : as the separator.
ipv6 address/port separation is not really clear.
Could you print RADDR and RPORT separately just like plain tccconnect.py does?

OK. Resubmitted.

@yonghong-song
Copy link
Collaborator

Looks good. Thanks!

@yonghong-song yonghong-song merged commit 9518a5b into iovisor:master Aug 1, 2019
@boat0 boat0 deleted the tcpconnect branch September 24, 2020 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants