[receiver/tcplog] Add metrics to track payload size and connections created/closed#45204
[receiver/tcplog] Add metrics to track payload size and connections created/closed#45204anubhav21sharma wants to merge 2 commits into
Conversation
thompson-tomo
left a comment
There was a problem hiding this comment.
It would be nice if these were generic collector metrics, similar to https://opentelemetry.io/docs/collector/internal-telemetry/
| if c.Metrics.Enabled { | ||
| meter := set.MeterProvider.Meter("github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/operator/input/tcp") | ||
| if metricPayloadSize, err = meter.Int64Histogram( | ||
| "otelcol_tcplog_receiver_payload_size_bytes", |
There was a problem hiding this comment.
| "otelcol_tcplog_receiver_payload_size_bytes", | |
| "otelcol_receiver_payload_size_bytes", |
Or to follow semantic conventions on namespacing:
| "otelcol_tcplog_receiver_payload_size_bytes", | |
| "otel.col.receiver.payload.size", |
Or following additional semconv patterns
| "otelcol_tcplog_receiver_payload_size_bytes", | |
| "otel.col.receiver.network.io", |
We could even have if we add otel.component.type attribute capturing that it is a collector reciever.
| "otelcol_tcplog_receiver_payload_size_bytes", | |
| "otel.col.network.io", |
All options use attributes to identify the reciever ie otel.component.name as well as the network direction.
| } | ||
|
|
||
| if metricConnectionsCreated, err = meter.Int64Counter( | ||
| "otelcol_tcplog_receiver_connections_created_total", |
There was a problem hiding this comment.
| "otelcol_tcplog_receiver_connections_created_total", | |
| "otelcol_receiver_connections_created_total", |
Or to follow semantic conventions on namespacing:
| "otelcol_tcplog_receiver_connections_created_total", | |
| "otel.col.receiver.network.connection.created", |
Or even which fits better with other conventions and allows consolidation.
| "otelcol_tcplog_receiver_connections_created_total", | |
| "otel.col.receiver.network.connection.status", |
With network.connection.state attribute = established
We could even have if we add otel.component.type attribute capturing that it is a collector reciever.
| "otelcol_tcplog_receiver_connections_created_total", | |
| "otel.col.network.connection.status", |
All options use attributes to identify the reciever ie otel.component.name
| } | ||
|
|
||
| if metricConnectionsClosed, err = meter.Int64Counter( | ||
| "otelcol_tcplog_receiver_connections_closed_total", |
There was a problem hiding this comment.
| "otelcol_tcplog_receiver_connections_closed_total", | |
| "otelcol_receiver_connections_created_total", |
Or to follow semantic conventions on namespacing:
| "otelcol_tcplog_receiver_connections_closed_total", | |
| "otel.col.receiver.network.connection.closed", |
Or even which fits better with other conventions and allows consolidation.
| "otelcol_tcplog_receiver_connections_closed_total", | |
| "otel.col.receiver.network.connection.status", |
With network.connection.state attribute = closed
We could even have if we add otel.component.type attribute capturing that it is a collector reciever.
| "otelcol_tcplog_receiver_connections_closed_total", | |
| "otel.col.network.connection.status", |
All options use attributes to identify the reciever ie otel.component.name
Thank you @thompson-tomo for your inputs! Do you mean to create these as standard metrics that are available for all receivers, i.e. add them as metrics to the Please let me know if I misunderstood this. Thank you! |
|
That is correct. Note the following rfc open-telemetry/opentelemetry-collector#11406 which provide more guidance on naming. |
|
I appreciate the input on naming conventions. I will change the metric names and attributes as per the RFC. About the standardization of these metrics, I’m a bit hesitant to treat these as 'general' receiver metrics because in my opinion they don't map well to receivers that do not do any network I/O. For example, many receivers like I'm relatively new to this codebase and would definitely rely on the judgement of more experienced folks here but I'm slightly leaning towards doing this change only for the But if we decide to move forward with the standardization of these metrics, I believe we need to cancel this PR and open a new issue in the core opentelemetry collector repo to discuss this further. Please let me know your thoughts. Really appreciate all your inputs! |
|
While I agree with you network namespace it is not applicable to all recievers/components I do however think it is broadly applicable to a large number of them hence prefer to facilitate reuse rather than duplication. Looking at the rfc, we could do the following:
It would be good if we kicked off a discussion about aligning internal telemetry definitions with semconv as that would resolve the naming question in point 1. |
|
Sounds fair. I'll cancel this PR then and raise a new issue to discuss this in the opentelemetry-collector repo. |
Description
This PR adds three new metrics to the tcplog receiver (opt-in) that allows users to track the number of new tcp connections created, number of tcp connections closed and the size distribution of the incoming payload to the receiver.
Metrics from a sample run:
Link to tracking issue
Fixes 45146
Testing
New test cases added to ensure metrics are being correctly populated.