-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: track lines rejected in prometheus metrics #25722
Conversation
This adds the metric `influxdb3_write_lines_rejected` metric which tracks the total number of lines rejected from incoming writes. Note, that this only tacks the number of rejected lines when the default `accept_partial` of `true` is provided to incoming write requests.
We could also probably track rejected lines in requests that are made with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve so you can get through, but just a few comments on the naming conventions.
@@ -4,37 +4,51 @@ use metric::{Metric, Registry, U64Counter}; | |||
|
|||
#[derive(Debug)] | |||
pub(super) struct WriteMetrics { | |||
write_lines_total: Metric<U64Counter>, | |||
write_bytes_total: Metric<U64Counter>, | |||
write_lines_count: Metric<U64Counter>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
write_lines_total
is the more standard naming convention. Metric names should end in units, e.g. _seconds, _bytes, or _total for unit-less metrics. See https://prometheus.io/docs/practices/naming/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed this in cd51bc2
pub(super) const WRITE_LINES_TOTAL_NAME: &str = "influxdb3_write_lines_total"; | ||
pub(super) const WRITE_BYTES_TOTAL_NAME: &str = "influxdb3_write_bytes_total"; | ||
pub(super) const WRITE_LINES_METRIC_NAME: &str = "influxdb3_write_lines"; | ||
pub(super) const WRITE_LINES_REJECTED_METRIC_NAME: &str = "influxdb3_write_lines_rejected"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should end in _total
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this in the PR description, but our Prometheus exporter adds a "_total" to the end of metric names that use a counter on export, which is why I removed it from the names here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I should have read that closer!
This adds the metric
influxdb3_write_lines_rejected_total
metric which tracks the total number of lines rejected from incoming writes.Note, that this only tacks the number of rejected lines when the default
accept_partial
oftrue
is provided to incoming write requests.This also changes the names of the
influxdb3_write_lines_total
andinfluxdb3_write_bytes_total
internally toinfluxdb3_write_lines
andinfluxdb3_write_bytes
, respectively. Since the Prometheus exporter adds the_total
for counter metrics for us see here, these were previously being exported asinfluxdb3_write_lines_total_total
andinfluxdb3_write_bytes_total_total
- FYI @saraghds.Closes #25696