feat(host_metrics source): add a new collector for tcp stats #22057

aryan9600 · 2024-12-18T17:43:30Z

Summary

Add a new tcp collector to the host_metrics source to expose information about the systems's TCP stack. It exposes three metrics:

tcp_connections_total: The total number of TCP connections. It includes the state of the connection as a tag.
tcp_tx_queued_bytes_total: The sum of the number of bytes in the send queue across all connections.
tcp_rx_queued_bytes_total: The sum of the number of bytes in the receive queue across all connections.

These metrics are only available for Linux systems as it uses the netlink subsystem.

Change Type

Bug fix
New feature
Non-functional (chore, refactoring, docs)
Performance

Is this a breaking change?

Yes
No

How did you test this PR?

Unit tests for the code interacting with netlink and for the code writing to the metrics buffer.
Manual testing by running the change locally with the below config file and comparing the output with ss -a -t on both Linux x86 and arm systems.

sources:
  host_metrics:
    type: host_metrics
    collectors:
      - tcp
sinks:
  console:
    type: console
    inputs:
      - host_metrics
    encoding:
      codec: text

Does this PR include user facing changes?

Yes. Please add a changelog fragment based on our guidelines.
No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

Please read our Vector contributor resources.
If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

closes #21972

bits-bot · 2024-12-18T17:43:35Z

All committers have signed the CLA.

jszwedko

Thanks @aryan9600 ! Exposing these new metrics seem useful. What do you think of putting them under the existing network collector though? Exporting as network_tcp_*.

aryan9600 · 2024-12-19T06:23:13Z

What do you think of putting them under the existing network collector though?

i wanted to keep it as a separate collector, as these metrics are only available for linux. also, to avoid any changes to the output of users when they upgrade to the newer version with these changes. if its a blocker, then i can move it to the network collector, no issue. let me know :)

jszwedko · 2024-12-19T21:56:30Z

What do you think of putting them under the existing network collector though?

i wanted to keep it as a separate collector, as these metrics are only available for linux. also, to avoid any changes to the output of users when they upgrade to the newer version with these changes. if its a blocker, then i can move it to the network collector, no issue. let me know :)

I think we already have a precedent of collectors having OS-specific metrics (e.g. filesystem_used_ratio only exists for Linux) so I think they'd be a good fit under collector even if they only exist for Linux.

pront

Hi @aryan9600, thank you for this PR!

I noticed big change in Cargo.lock, I wonder if you can roll that file back and apply individual updates for the newly added crates only.

aryan9600 · 2025-01-06T13:48:27Z

a problem with having these TCP metrics inside the network collector is that the latter allows to filter the output metrics using network devices, but the former doesn't support this filtering. this is due to the fact that netlink doesn't return the network interface in the response for SOCK_DIAG messages. we could modify the tests and docs to accommodate for this or create a new collector for these metrics. open to any other suggestions.
cc: @pront @jszwedko

pront · 2025-01-06T14:21:37Z

Hi @aryan9600, one test is failing. I will take another look at this PR once that is fixed. You can run any test locally with cargo test.

pront

/ci-run-unit-mac

pront · 2025-01-07T16:57:30Z

Running this locally on macOS I get multiple compilation errors originating netlink-packet-sock-diag.

error[E0425]: cannot find value `AF_AX25` in crate `libc`
  --> .../.cargo/registry/src/index.crates.io-6f17d22bba15001f/netlink-packet-sock-diag-0.4.2/src/constants.rs:10:31
   |
10 | pub const AF_AX25: u8 = libc::AF_AX25 as u8;
   |                               ^^^^^^^ not found in `libc`

jszwedko · 2025-01-07T18:57:50Z

a problem with having these TCP metrics inside the network collector is that the latter allows to filter the output metrics using network devices, but the former doesn't support this filtering. this is due to the fact that netlink doesn't return the network interface in the response for SOCK_DIAG messages. we could modify the tests and docs to accommodate for this or create a new collector for these metrics. open to any other suggestions.

Ah, good point, that could be actually be a reason to separate the collector if it is not filterable by network device.

aryan9600 · 2025-01-08T11:48:54Z

@jszwedko ack, moving these metrics back into its own dedicated collector.

Add a new `tcp` collector to the `host_metrics` source to expose information about the systems's TCP stack. It exposes three metrics: * `tcp_connections_total`: The total number of TCP connections. It includes the `state` of the connection as a tag. * `tcp_tx_queued_bytes_total`: The sum of the number of bytes in the send queue across all connections. * `tcp_rx_queued_bytes_total`: The sum of the number of bytes in the receive queue across all connections. The collector is only enabled for Linux as it uses the netlink subsystem. Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

aryan9600 · 2025-01-08T17:53:40Z

Running this locally on macOS I get multiple compilation errors originating netlink-packet-sock-diag.

@pront this should be fixed.

aryan9600 · 2025-01-09T05:12:31Z

tests are passing now, previous CI run failed because third party licenses were not up to date. this has been fixed now.

jszwedko

👍 on the UX. I'll leave the code review to @pront

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

pront

Couple of comments on diagnostics but otherwise this is good to go 🚀

src/sources/host_metrics/tcp.rs

pront · 2025-01-09T20:20:43Z

src/sources/host_metrics/tcp.rs

+            9 => Ok(TcpState::LastAck),
+            10 => Ok(TcpState::Listen),
+            11 => Ok(TcpState::Closing),
+            _ => Err(TcpError::InvalidTcpState { state: value }),


Nit: we could pass the string representation of state here

i'm a bit confused, what would be the string representation of state here, since value doesn't correspond to a valid tcp state?

Ah my bad, I see that this is a catch all for states that don't map to a string.

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

pront

Doing one last check on my side because last time we tried to generate linux specific platform docs it broke component generation on macOS. This something for us (Vector team) to figure out. PR looks great otherwise.

pront · 2025-01-10T14:45:56Z

src/sources/host_metrics/tcp.rs

+            9 => Ok(TcpState::LastAck),
+            10 => Ok(TcpState::Listen),
+            11 => Ok(TcpState::Closing),
+            _ => Err(TcpError::InvalidTcpState { state: value }),


Ah my bad, I see that this is a catch all for states that don't map to a string.

pront · 2025-01-10T15:12:31Z

make generate-component-docs on macOS:

Raw: diff.txt

@jszwedko: do you happen to recall how we dealt with this in the past?

jszwedko · 2025-01-10T17:21:45Z

make generate-component-docs on macOS:
Raw: [diff.txt](https://github.com/user-attachments/files/18378930/diff.txt)
@jszwedko: do you happen to recall how we dealt with this in the past?

I think maybe the cue files were manually modified rather than using make generate-component-docs. I believe the diff you have is accurate and should be applied 🤔

jszwedko · 2025-01-10T17:22:42Z

Though I do see the "Only available on Linux" bit of the doc comment is dropped. That might be an issue in the scaffolding that generates the cue file based on the structs.

pront · 2025-01-10T17:25:22Z

make generate-component-docs on macOS:

Raw: diff.txt
@jszwedko: do you happen to recall how we dealt with this in the past?

I think maybe the cue files were manually modified rather than using make generate-component-docs. I believe the diff you have is accurate and should be applied 🤔

Thanks @jszwedko. @aryan9600 please take a look at the above.

pront · 2025-01-10T17:25:53Z

Though I do see the "Only available on Linux" bit of the doc comment is dropped. That might be an issue in the scaffolding that generates the cue file based on the structs.

I will take a look at this.

aryan9600 requested review from a team as code owners December 18, 2024 17:43

github-actions bot added domain: sources Anything related to the Vector's sources domain: external docs Anything related to Vector's external, public documentation labels Dec 18, 2024

aryan9600 force-pushed the host-metrics-tcp branch from 6efd703 to 32a5fe8 Compare December 18, 2024 17:46

rtrieu approved these changes Dec 18, 2024

View reviewed changes

jszwedko reviewed Dec 18, 2024

View reviewed changes

aryan9600 changed the title ~~feat(host_metrics): add a new tcp collector~~ feat(host_metrics): modify the network collector to expose TCP stats Dec 20, 2024

aryan9600 requested a review from jszwedko December 20, 2024 19:09

aryan9600 force-pushed the host-metrics-tcp branch from 39bc32d to bdb5596 Compare December 20, 2024 19:15

aryan9600 changed the title ~~feat(host_metrics): modify the network collector to expose TCP stats~~ feat(host_metrics source): modify the network collector to expose TCP stats Dec 21, 2024

aryan9600 force-pushed the host-metrics-tcp branch from 7392558 to 0bcdd70 Compare December 23, 2024 06:01

pront reviewed Jan 2, 2025

View reviewed changes

aryan9600 force-pushed the host-metrics-tcp branch from 0bcdd70 to 9faccc7 Compare January 3, 2025 08:05

aryan9600 requested a review from pront January 3, 2025 13:34

pront self-assigned this Jan 3, 2025

pront reviewed Jan 7, 2025

View reviewed changes

pront force-pushed the host-metrics-tcp branch from 6419d0b to 9faccc7 Compare January 7, 2025 16:45

aryan9600 added 2 commits January 8, 2025 18:59

chore(host_metrics): add changelog fragment for tcp collector

4cafa88

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

aryan9600 force-pushed the host-metrics-tcp branch from 9faccc7 to 53d380a Compare January 8, 2025 17:53

aryan9600 requested a review from pront January 8, 2025 17:58

aryan9600 changed the title ~~feat(host_metrics source): modify the network collector to expose TCP stats~~ feat(host_metrics source): add a new collector for tcp stats Jan 9, 2025

jszwedko approved these changes Jan 9, 2025

View reviewed changes

aryan9600 added 2 commits January 9, 2025 23:46

chore(host_metrics): add docs for tcp collector

63cf9b0

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

chore: update third party licenses

114e3e0

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

aryan9600 force-pushed the host-metrics-tcp branch from 964de55 to 114e3e0 Compare January 9, 2025 18:17

pront approved these changes Jan 9, 2025

View reviewed changes

chore(host_metrics): include error source in snafu display msg

3dd11e6

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

pront reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(host_metrics source): add a new collector for tcp stats #22057

feat(host_metrics source): add a new collector for tcp stats #22057

aryan9600 commented Dec 18, 2024 •

edited

Loading

bits-bot commented Dec 18, 2024 •

edited

Loading

jszwedko left a comment

aryan9600 commented Dec 19, 2024

jszwedko commented Dec 19, 2024

pront left a comment

aryan9600 commented Jan 6, 2025

pront commented Jan 6, 2025

pront left a comment

pront commented Jan 7, 2025

jszwedko commented Jan 7, 2025

aryan9600 commented Jan 8, 2025

aryan9600 commented Jan 8, 2025

aryan9600 commented Jan 9, 2025

jszwedko left a comment

pront left a comment

pront Jan 9, 2025

aryan9600 Jan 10, 2025

pront Jan 10, 2025

pront left a comment

pront Jan 10, 2025

pront commented Jan 10, 2025 •

edited

Loading

jszwedko commented Jan 10, 2025

jszwedko commented Jan 10, 2025

pront commented Jan 10, 2025

pront commented Jan 10, 2025

feat(host_metrics source): add a new collector for tcp stats #22057

Are you sure you want to change the base?

feat(host_metrics source): add a new collector for tcp stats #22057

Conversation

aryan9600 commented Dec 18, 2024 • edited Loading

Summary

Change Type

Is this a breaking change?

How did you test this PR?

Does this PR include user facing changes?

Checklist

References

bits-bot commented Dec 18, 2024 • edited Loading

jszwedko left a comment

Choose a reason for hiding this comment

aryan9600 commented Dec 19, 2024

jszwedko commented Dec 19, 2024

pront left a comment

Choose a reason for hiding this comment

aryan9600 commented Jan 6, 2025

pront commented Jan 6, 2025

pront left a comment

Choose a reason for hiding this comment

pront commented Jan 7, 2025

jszwedko commented Jan 7, 2025

aryan9600 commented Jan 8, 2025

aryan9600 commented Jan 8, 2025

aryan9600 commented Jan 9, 2025

jszwedko left a comment

Choose a reason for hiding this comment

pront left a comment

Choose a reason for hiding this comment

pront Jan 9, 2025

Choose a reason for hiding this comment

aryan9600 Jan 10, 2025

Choose a reason for hiding this comment

pront Jan 10, 2025

Choose a reason for hiding this comment

pront left a comment

Choose a reason for hiding this comment

pront Jan 10, 2025

Choose a reason for hiding this comment

pront commented Jan 10, 2025 • edited Loading

jszwedko commented Jan 10, 2025

jszwedko commented Jan 10, 2025

pront commented Jan 10, 2025

pront commented Jan 10, 2025

aryan9600 commented Dec 18, 2024 •

edited

Loading

bits-bot commented Dec 18, 2024 •

edited

Loading

pront commented Jan 10, 2025 •

edited

Loading