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

Metrics request bytes #4982

Closed
wants to merge 2 commits into from

Conversation

NyaliaLui
Copy link
Contributor

Cover letter coming soon.

Add topic level metric, intended for the new endpoint, that measure the number of bytes in client payloads by request type.

return;
}

std::vector<sm::label_instance> labels{sm::label("request")("produce|consume")};
Copy link
Contributor

Choose a reason for hiding this comment

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

When I read the metric specification (see below) I interpreted it to mean that the request label should take either the produce or consume value depending on the request being processed. To achieve this you'd need to have two metrics in the add_group call: one with the consume label and the other with the produce label.

# HELP redpanda_kafka_request_bytes_total Number of bytes in client payloads by request type
# TYPE redpanda_kafka_request_bytes_total counter
redpanda_kafka_request_bytes_total{request="produce|consume",namespace,topic}

@@ -192,6 +196,8 @@ class request_context {
request_header _header;
request_reader _reader;
ss::lowres_clock::duration _throttle_delay;

kafka::topic_probe _topic_probe;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the lifetime of request context objects is roughly the same as the lifetime of a request in the system, which means that it will get destroyed when request processing is complete. The ss::metrics::metric_groups destructor (owned by the probe) removes all the registered metrics so they're lost after each request.

The probe should probably be stashed somewhere else (to match the lifetime of the server maybe).

@NyaliaLui NyaliaLui force-pushed the metrics-request-bytes branch from 1217149 to 1879fdb Compare June 3, 2022 16:12
@@ -186,6 +186,8 @@ static partition_produce_stages partition_append(
p.error_code = error_code::none;
partition->probe().add_records_produced(num_records);
partition->probe().add_bytes_produced(num_bytes);
partition->probe_v2().add_produce_bytes_cluster_lvl(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The requirements for this new metric are a combination of the old metrics generated from partition_probe::add_bytes_produced() and partition_probe::add_bytes_fetched(). But those metrics were at the cluster level. I still need to figure out where to put the topic level metrics.

@NyaliaLui
Copy link
Contributor Author

Closing since this was implemented in #5165

@NyaliaLui NyaliaLui closed this Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants