some metrics for measuring performance and failures in boltdb shipper #2034
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2034 +/- ##
==========================================
- Coverage 64.05% 64.03% -0.02%
==========================================
Files 133 134 +1
Lines 10217 10296 +79
==========================================
+ Hits 6544 6593 +49
- Misses 3185 3210 +25
- Partials 488 493 +5
|
pkg/storage/stores/local/shipper.go
Outdated
| @@ -235,6 +243,7 @@ func (s *Shipper) syncLocalWithStorage(ctx context.Context) error { | |||
|
|
|||
| for period := range s.downloadedPeriods { | |||
| if err := s.syncFilesForPeriod(ctx, period, s.downloadedPeriods[period]); err != nil { | |||
| s.metrics.filesDownloadFailures.Inc() | |||
There was a problem hiding this comment.
Why don't you track success and failure via the same counter ? I think it gives better possibilities.
pkg/storage/stores/local/shipper.go
Outdated
| level.Error(pkg_util.Logger).Log("msg", "error syncing local boltdb files with storage", "err", err) | ||
| } | ||
| s.metrics.filesDownloadOperationTotal.WithLabelValues(status).Inc() |
There was a problem hiding this comment.
a defer could do a better job but that's fine !
There was a problem hiding this comment.
I agree with @cyriltovena here -- defer would help with some code deduplication.
pkg/storage/stores/local/metrics.go
Outdated
|
|
||
| type boltDBShipperMetrics struct { | ||
| // metrics for measuring performance of downloading of files per period initially i.e for the first time | ||
| initialFilesDownloadDurationSeconds *prometheus.GaugeVec |
There was a problem hiding this comment.
I'm not sure if these are good metrics - won't they be too irregular? I expect it'll be hard to query these in prom if they're not regularly populated. I wonder if gauges which aren't updated regularly are still scraped regularly, hrm...
Also I'm wary of creating labels based on the table name.
There was a problem hiding this comment.
I agree that these metrics are irregular but they are there just to help understand slow queries which would most likely happen when downloading large index file. Without this metric we would have to rely on logs to find if query is slow because of downloading of files or some other contention is there.
Regarding labels based on table names, there would not be too many tables at a time. Also without that label, we would not have much visibility because otherwise the same metric would be updated for all the table downloads and the last one which was updated before prometheus scrapes would be visible, which would not be much of a use. What do you think?
There was a problem hiding this comment.
The other we have is adding a span in traces and getting rid of this metric.
There was a problem hiding this comment.
My concerns about sparsely populated gauges have been alleviated (this is handled via our instrumentation lib).
However, it seems weird to be creating per-table gauges that are only written on startup/first pull. This is even more costly if tables are/eventually become per-tenant. Could this instead be calculated across all tables? I find it hard to imagine needing this level of granularity: we'll be pulling tables from the same object stores. I suspect having one gauge across all tables will be enough to help us and if we need to find one problematic table, we can use logs (we'll need to ensure per-table downloads are logged).
Did I explain that well?
There was a problem hiding this comment.
Yes, I understand your concern. The problem here is IndexClients get per table query so we can't have a metric across all tables downloaded for a query. Best option we have here is adding a span to the trace.
There was a problem hiding this comment.
I was thinking some shared struct which keeps individual download times/bytes for each table, then updates one gauge whenever a new table is downloaded. The gauge can be calculated from the sum of download times/bytes from each table.
There was a problem hiding this comment.
Something like
type DownloadTableMetrics struct{
gauge *prometheus.Gauge
tables map[string]struct{
dur time.Duration
bytes int
}
}Then when downloading a table you can do
downloadTableMetrics.Add(tableName, downloadTime, byteCt), which will internally add that value then recalculate/update its underlying gauge.
pkg/storage/stores/local/shipper.go
Outdated
| level.Error(pkg_util.Logger).Log("msg", "error syncing local boltdb files with storage", "err", err) | ||
| } | ||
| s.metrics.filesDownloadOperationTotal.WithLabelValues(status).Inc() |
There was a problem hiding this comment.
I agree with @cyriltovena here -- defer would help with some code deduplication.
|
@owen-d I'll you take care of merging this. |
What this PR does / why we need it:
Adds following metrics for measuring performance and failures in boltdb shipper.
loki_boltdb_shipper_files_download_failures_total: Total number of failures in downloading files from shared store.loki_boltdb_shipper_files_upload_failures_total: Total number of failures in uploading files from shared store.loki_boltdb_shipper_initial_files_download_duration_seconds: Measures download time per period only for initial download since that is what would impact query performance.loki_boltdb_shipper_initial_files_download_size_bytes: Measures total files size per period only for initial download since that is what would impact query performance.loki_boltdb_shipper_request_duration_seconds: Histogram measuring time(in seconds) spent in processing requests. We do only read requests for now.