Conversation
collector/tcpstat.go
Outdated
There was a problem hiding this comment.
/proc/net/tcp is n^3, if you need this information you need to use the same syscalls ss(1) does.
There was a problem hiding this comment.
ss uses Netlink sockets under the hood. From strace:
Request for data:
socket(PF_NETLINK, SOCK_RAW, 4) = 3
sendmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{"L\0\0\0\22\0\1\3@\342\1\0\0\0\0\0\2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 76}], msg_controllen=0, msg_flags=0}, 0) = 76
Reading data:
recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, msg_iov(1)=[{"`\0\0\0\22\0\2\0@\342\1\0\2127\0\0\n\10\0\0\2408\2w\0\0\0\0\0\0\0\0"..., 8192}], msg_controllen=0, msg_flags=0}, 0) = 1344
Go seems to support Netlink (http://golang.org/pkg/syscall), so this should be doable... let me know if I can help build that code.
There was a problem hiding this comment.
When I looked into this a few years back the API ss used wasn't very mature. I ended up shelling out to ss on that occasion, which isn't an option here.
There was a problem hiding this comment.
Wasn't mature in what sense? It was undergoing breaking changes (unlikely with kernel interfaces), or it didn't work correctly yet? Or just not available in older kernels?
There was a problem hiding this comment.
It seemed more of a thing to get just ss(1) working, rather than a generic solution with library support.
There was a problem hiding this comment.
Sounds to me like reading /proc is the better option at least until it proves necessary to optimize further?
There was a problem hiding this comment.
@matthiasr Yeah, the only problem is that it might accidentally really slow down people's node exporters or machines especially in high-load situations (tens of thousands of connections).
There was a problem hiding this comment.
Ok, let's use the /proc/net/tcp support until someone manages to write Netlink code, but let's not enable this collector by default (that's already the case, so no change needed regarding that).
|
👍 besides the minor style nits and the question about whether to put IPv4/IPv6 into a label. Thanks for bearing with us! |
There was a problem hiding this comment.
Sorry, I have added modification. (93d21b5)
Fix always error on not exists /proc/net/tcp6.
|
@kjmkznr Cool, looks good to me for now, and we can still add extra labels later or improve the way how we get the TCP stats (netlink). Could you squash your commits? Then I'll merge. |
|
The readme needs to be updated too, with a warning about the performance issue. |
|
Good point. |
|
Update readme. |
README.md
Outdated
There was a problem hiding this comment.
Some minor things here, can we do:
"Exposes TCP connection status information from /proc/net/tcp and /proc/net/tcp6. (Warning: the current version has potential performance issues in high load situations.)"
|
@kjmkznr Hm, why would you create a new PR? Usually you can just squash the commits in the existing PR. |
|
@juliusv Can I force push to current branch? |
|
Yup! |
kjmkznr
force-pushed
the
add-tcp-stat-collector
branch
from
f654b16 to
30280d5
Compare
|
I didn't know github spec. Thanks! |
|
@kjmkznr Cool, could you just still integrate this comment before merging? https://github.com/prometheus/node_exporter/pull/56/files#r27012081 |
kjmkznr
force-pushed
the
add-tcp-stat-collector
branch
from
30280d5 to
e4da771
Compare
|
I overlooked that. |
|
@kjmkznr Thanks a bunch! |
|
Thanks!
|
Replace fixtures.tar.gz with regular files
The generated pathnames are too long for some filenames, including NTFS in its default configuration. We've considered using other separators, but deemed shorter ones to easy to break something. This reverts commit b9955ae, reversing changes made to a3bfc74. Fixes prometheus#57.
4 participants
This collector expose following metrics: