-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add sync metrics #533
Add sync metrics #533
Conversation
src/frontend/flight/sync/metrics.rs
Outdated
const REQUEST: &str = "seafowl_sync_writer_request"; | ||
const IN_MEMORY: &str = "seafowl_sync_writer_in_memory"; | ||
const COMPACTION_TIME: &str = "seafowl_sync_writer_compaction_time"; | ||
const COMPACTED: &str = "seafowl_sync_writer_compacted"; | ||
const FLUSHING_TIME: &str = "seafowl_sync_writer_flushing_time"; | ||
const FLUSHED: &str = "seafowl_sync_writer_flushed"; |
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.
https://prometheus.io/docs/practices/naming/
The rule of thumb for these is that if you sum up some metric by all possible label values, it should still be a meaningful value, so we can't have different units in the same metric, instead:
# counter: total rows requested and total size of requests
seafowl_sync_writer_requests_rows_total
seafowl_sync_writer_requests_bytes_total
# gauges: how many rows in memory and how much space do they take
seafowl_sync_writer_in_memory_rows_total
seafowl_sync_writer_in_memory_bytes_total
# histogram: time taken by compaction
seafowl_sync_writer_compaction_time_seconds
# counter: total rows compacted, amount and size
seafowl_sync_writer_compacted_rows_total
seafowl_sync_writer_compacted_bytes_total
# histogram: time taken by flushes
seafowl_sync_writer_flushed_time_seconds
# counter: total rows flushed, amount and size
seafowl_sync_writer_flushed_bytes_total
seafowl_sync_writer_flushed_rows_total
src/frontend/flight/sync/metrics.rs
Outdated
Gauge, Histogram, | ||
}; | ||
|
||
const REQUEST: &str = "seafowl_sync_writer_request"; |
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 think we should rename this sync
to something else, seafowl_delta_writer
? seafowl_writer
? seafowl_changeset_writer
?
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 go with seafowl_changeset_writer
.
src/frontend/flight/sync/metrics.rs
Outdated
const REQUEST_ROWS: &str = "seafowl_changeset_writer_request_rows"; | ||
const IN_MEMORY_BYTES: &str = "seafowl_changeset_writer_in_memory_bytes"; | ||
const IN_MEMORY_ROWS: &str = "seafowl_changeset_writer_in_memory_rows"; | ||
const IN_MEMORY_OLDEST: &str = "seafowl_changeset_writer_in_memory_oldest"; |
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.
_oldest_timestamp_seconds
?
src/frontend/flight/sync/metrics.rs
Outdated
"The reduction in byte size due to batch compaction" | ||
); | ||
describe_counter!( | ||
COMPACTED_BYTES, |
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.
COMPACTED_BYTES, | |
COMPACTED_ROWS, |
Add metrics tracking byte size/row count for:
as well as timings for the last two ops.