[receiver/hostmetrics] add udp connections #11046

codeboten · 2022-06-14T23:34:25Z

This change adds a metric for UDP connections system.network.udp.connections.

As part of this I've renamed system.network.connections to system.network.tcp.connections as it wasn't clear to me that UDP and TCP provide any value when combined. Additionally, the state attribute didn't make sense for UDP connections.

dmitryax · 2022-06-15T00:18:42Z

Since this is part of the specification https://github.com/open-telemetry/opentelemetry-specification/blob/7e35bec9401d9bc970a7107af45906c0890040b6/specification/metrics/semantic_conventions/system-metrics.md I believe we have to update the spec first.

dmitryax · 2022-06-15T00:22:32Z

The spec defines protocol attribute. I can image combining udp+tcp to be useful. If you think otherwise, let's submit a change to the spec first?

receiver/hostmetricsreceiver/internal/scraper/networkscraper/network_scraper.go

codeboten · 2022-06-15T15:06:47Z

The spec defines protocol attribute. I can image combining udp+tcp to be useful. If you think otherwise, let's submit a change to the spec first?

Yes I guess I was thinking about this in the context of the direction discussion and started to question whether it makes sense to combine metrics for separate protocols.

As a comparison point, the node exporter in Prometheus separates netstat metrics per protocol:

# HELP node_netstat_TcpExt_SyncookiesSent Statistic TcpExtSyncookiesSent.
# TYPE node_netstat_TcpExt_SyncookiesSent untyped
node_netstat_TcpExt_SyncookiesSent 0
# HELP node_netstat_TcpExt_TCPSynRetrans Statistic TcpExtTCPSynRetrans.
# TYPE node_netstat_TcpExt_TCPSynRetrans untyped
node_netstat_TcpExt_TCPSynRetrans 0
# HELP node_netstat_TcpExt_TCPTimeouts Statistic TcpExtTCPTimeouts.
# TYPE node_netstat_TcpExt_TCPTimeouts untyped
node_netstat_TcpExt_TCPTimeouts 0
# HELP node_netstat_Tcp_ActiveOpens Statistic TcpActiveOpens.
# TYPE node_netstat_Tcp_ActiveOpens untyped
node_netstat_Tcp_ActiveOpens 0
# HELP node_netstat_Tcp_CurrEstab Statistic TcpCurrEstab.
# TYPE node_netstat_Tcp_CurrEstab untyped
node_netstat_Tcp_CurrEstab 1
# HELP node_netstat_Tcp_InErrs Statistic TcpInErrs.
# TYPE node_netstat_Tcp_InErrs untyped
node_netstat_Tcp_InErrs 0
# HELP node_netstat_Tcp_InSegs Statistic TcpInSegs.
# TYPE node_netstat_Tcp_InSegs untyped
node_netstat_Tcp_InSegs 105
# HELP node_netstat_Tcp_OutRsts Statistic TcpOutRsts.
# TYPE node_netstat_Tcp_OutRsts untyped
node_netstat_Tcp_OutRsts 0
# HELP node_netstat_Tcp_OutSegs Statistic TcpOutSegs.
# TYPE node_netstat_Tcp_OutSegs untyped
node_netstat_Tcp_OutSegs 241
# HELP node_netstat_Tcp_PassiveOpens Statistic TcpPassiveOpens.
# TYPE node_netstat_Tcp_PassiveOpens untyped
node_netstat_Tcp_PassiveOpens 6
# HELP node_netstat_Tcp_RetransSegs Statistic TcpRetransSegs.
# TYPE node_netstat_Tcp_RetransSegs untyped
node_netstat_Tcp_RetransSegs 0
# HELP node_netstat_Udp6_InDatagrams Statistic Udp6InDatagrams.
# TYPE node_netstat_Udp6_InDatagrams untyped
node_netstat_Udp6_InDatagrams 0
# HELP node_netstat_Udp6_InErrors Statistic Udp6InErrors.
# TYPE node_netstat_Udp6_InErrors untyped
node_netstat_Udp6_InErrors 0
# HELP node_netstat_Udp6_NoPorts Statistic Udp6NoPorts.
# TYPE node_netstat_Udp6_NoPorts untyped
node_netstat_Udp6_NoPorts 0
# HELP node_netstat_Udp6_OutDatagrams Statistic Udp6OutDatagrams.
# TYPE node_netstat_Udp6_OutDatagrams untyped
node_netstat_Udp6_OutDatagrams 0
# HELP node_netstat_Udp6_RcvbufErrors Statistic Udp6RcvbufErrors.
# TYPE node_netstat_Udp6_RcvbufErrors untyped
node_netstat_Udp6_RcvbufErrors 0
# HELP node_netstat_Udp6_SndbufErrors Statistic Udp6SndbufErrors.
# TYPE node_netstat_Udp6_SndbufErrors untyped
node_netstat_Udp6_SndbufErrors 0
# HELP node_netstat_UdpLite6_InErrors Statistic UdpLite6InErrors.
# TYPE node_netstat_UdpLite6_InErrors untyped
node_netstat_UdpLite6_InErrors 0
# HELP node_netstat_UdpLite_InErrors Statistic UdpLiteInErrors.
# TYPE node_netstat_UdpLite_InErrors untyped
node_netstat_UdpLite_InErrors 0
# HELP node_netstat_Udp_InDatagrams Statistic UdpInDatagrams.
# TYPE node_netstat_Udp_InDatagrams untyped
node_netstat_Udp_InDatagrams 0
# HELP node_netstat_Udp_InErrors Statistic UdpInErrors.
# TYPE node_netstat_Udp_InErrors untyped
node_netstat_Udp_InErrors 0
# HELP node_netstat_Udp_NoPorts Statistic UdpNoPorts.
# TYPE node_netstat_Udp_NoPorts untyped
node_netstat_Udp_NoPorts 0
# HELP node_netstat_Udp_OutDatagrams Statistic UdpOutDatagrams.
# TYPE node_netstat_Udp_OutDatagrams untyped
node_netstat_Udp_OutDatagrams 0
# HELP node_netstat_Udp_RcvbufErrors Statistic UdpRcvbufErrors.
# TYPE node_netstat_Udp_RcvbufErrors untyped
node_netstat_Udp_RcvbufErrors 0

To be clear I don't have strong feelings one way or the other, just want to make sure we're doing the right thing for users.

dmitryax · 2022-06-15T16:01:27Z

Ok. I still think sum aggregation over protocol is "more" meaningful than direction. I would recommend keeping this change as a non-breaking addition with extra attribute value. Then we can raise this issue in the spec if you think protocol attribute should not be used

codeboten · 2022-06-15T17:51:46Z

@dmitryax i'm ok with this proposal. i've added a question around protocol attribute to the spec PR for addressing direction concerns.

Please take a look at this PR. A couple of questions:

should 0 be recorded for connections? we don't today for tcp
any thought around how attributes should be omitted for mdatagen code?

dmitryax · 2022-06-15T19:32:52Z

should 0 be recorded for connections? we don't today for tcp

I believe we shouldn't within scope of this change for consistency with TCP, we can revisit it later for both.

any thought around how attributes should be omitted for mdatagen code?

Sorry, I'm not sure I understand the question. Why do we need to omit any attribute? If an attribute is defined for a metric in metadata.yaml, it must be always emitted

dmitryax · 2022-06-15T19:38:40Z

receiver/hostmetricsreceiver/internal/scraper/networkscraper/network_scraper.go

+		return fmt.Errorf("failed to read UDP connections: %w", err)
+	}
+
+	// TODO: state attribute should be omitted for UDP connections


I understand your second question now. I answered before looking at the code. This seems like a good reason to separate metrics. Attribute values should not be dependent on each other. Can you please bring this problem to the spec issue for separating the metric. Maybe we should wait until that spec issue is resolved before merging this PR?

👍🏻 I'll see what the feedback from the spec to my question in this PR is open-telemetry/opentelemetry-specification#2617, please add yours there. I can open a separate issue if enough folks feel that separating the protocol makes sense.

Just chiming in to say that I agree that the initial approach of recording these as separate metrics makes the most sense.

github-actions · 2022-06-30T05:17:32Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2022-07-21T05:19:54Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

This change adds a metric for UDP connections. As part of this I've renamed `system.network.connections` to `system.network.tcp.connections` as it wasn't clear to me that UDP and TCP provide any value when combined. Additionally, the state attribute didn't make sense for UDP connections.

codeboten · 2022-08-02T19:31:48Z

Updated the code to use a feature gate to help users migrate to the metric w/o a protocol attribute like we did for direction. This PR will wait for open-telemetry/opentelemetry-specification#2675 to be merged before moving forward.

github-actions · 2022-08-21T05:16:34Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2022-09-18T05:30:03Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

codeboten requested a review from a team June 14, 2022 23:34

codeboten requested a review from dmitryax as a code owner June 14, 2022 23:34

github-actions bot assigned djaglowski Jun 14, 2022

dmitryax reviewed Jun 15, 2022

View reviewed changes

receiver/hostmetricsreceiver/internal/scraper/networkscraper/network_scraper.go Outdated Show resolved Hide resolved

codeboten force-pushed the codeboten/add-udp-connection branch from 2d6c179 to 385ea86 Compare June 15, 2022 14:47

dmitryax reviewed Jun 15, 2022

View reviewed changes

github-actions bot added the Stale label Jun 30, 2022

codeboten removed the Stale label Jul 6, 2022

github-actions bot added the Stale label Jul 21, 2022

codeboten removed the Stale label Jul 25, 2022

codeboten force-pushed the codeboten/add-udp-connection branch from 27d5874 to 3c08892 Compare August 2, 2022 19:30

github-actions bot added the Stale label Aug 21, 2022

github-actions bot closed this Sep 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[receiver/hostmetrics] add udp connections #11046

[receiver/hostmetrics] add udp connections #11046

codeboten commented Jun 14, 2022

dmitryax commented Jun 15, 2022

dmitryax commented Jun 15, 2022 •

edited

Loading

codeboten commented Jun 15, 2022

dmitryax commented Jun 15, 2022 •

edited

Loading

codeboten commented Jun 15, 2022

dmitryax commented Jun 15, 2022 •

edited

Loading

dmitryax Jun 15, 2022 •

edited

Loading

codeboten Jun 15, 2022

mwear Jun 15, 2022

github-actions bot commented Jun 30, 2022

github-actions bot commented Jul 21, 2022

codeboten commented Aug 2, 2022

github-actions bot commented Aug 21, 2022

github-actions bot commented Sep 18, 2022

[receiver/hostmetrics] add udp connections #11046

[receiver/hostmetrics] add udp connections #11046

Conversation

codeboten commented Jun 14, 2022

dmitryax commented Jun 15, 2022

dmitryax commented Jun 15, 2022 • edited Loading

codeboten commented Jun 15, 2022

dmitryax commented Jun 15, 2022 • edited Loading

codeboten commented Jun 15, 2022

dmitryax commented Jun 15, 2022 • edited Loading

dmitryax Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

codeboten Jun 15, 2022

Choose a reason for hiding this comment

mwear Jun 15, 2022

Choose a reason for hiding this comment

github-actions bot commented Jun 30, 2022

github-actions bot commented Jul 21, 2022

codeboten commented Aug 2, 2022

github-actions bot commented Aug 21, 2022

github-actions bot commented Sep 18, 2022

dmitryax commented Jun 15, 2022 •

edited

Loading

dmitryax commented Jun 15, 2022 •

edited

Loading

dmitryax commented Jun 15, 2022 •

edited

Loading

dmitryax Jun 15, 2022 •

edited

Loading