metrics for monitoring delete requests #2445
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Sandeep Sukhani <[email protected]>
pracucci
reviewed
Apr 14, 2020
pkg/chunk/purger/purger.go
Outdated
| }, []string{"user"}) | ||
| m.deleteRequestsProcessingFailures = prometheus.NewGaugeVec(prometheus.GaugeOpts{ | ||
| Namespace: "cortex", | ||
| Name: "purger_delete_requests_processing_failure", |
There was a problem hiding this comment.
What do you think to add an s to failure?
Suggested change
| Name: "purger_delete_requests_processing_failure", | |
| Name: "purger_delete_requests_processing_failures", |
pkg/chunk/purger/purger.go
Outdated
| func newPurgerMetrics(r prometheus.Registerer) *purgerMetrics { | ||
| m := purgerMetrics{} | ||
|
|
||
| m.deleteRequestsProcessedTotal = prometheus.NewCounterVec(prometheus.CounterOpts{ |
There was a problem hiding this comment.
Instead of calling r.MustRegister() below, could you create the metrics with promauto.With(r).NewCounterVec() (and same for NewGaugeVec())? It's the pattern we're heading to.
pkg/chunk/purger/request_handler.go
Outdated
| func newDeleteRequestHandlerMetrics(r prometheus.Registerer) *deleteRequestHandlerMetrics { | ||
| m := deleteRequestHandlerMetrics{} | ||
|
|
||
| m.deleteRequestsReceivedTotal = prometheus.NewCounterVec(prometheus.CounterOpts{ |
There was a problem hiding this comment.
promauto.With(r) here as well, please.
pkg/chunk/purger/purger.go
Outdated
| Name: "purger_delete_requests_chunks_selected_total", | ||
| Help: "Number of chunks selected while building delete plans per user", | ||
| }, []string{"user"}) | ||
| m.deleteRequestsProcessingFailures = prometheus.NewGaugeVec(prometheus.GaugeOpts{ |
There was a problem hiding this comment.
As discussed offline, this should be a counter.
Signed-off-by: Sandeep Sukhani <[email protected]>
pstibrany
approved these changes
Apr 17, 2020
| "strings" | ||
|
|
||
| "github.com/prometheus/client_golang/prometheus" | ||
|
|
There was a problem hiding this comment.
Remove new line please, and merge these two import groups.
pkg/chunk/purger/purger.go
Outdated
| }, []string{"user"}) | ||
| m.deleteRequestsProcessingFailures = promauto.With(r).NewCounterVec(prometheus.CounterOpts{ | ||
| Namespace: "cortex", | ||
| Name: "purger_delete_requests_processing_failures", |
There was a problem hiding this comment.
Missing _total suffix for counter. I don't like it, but that's what the convention is.
| // groups chunks together by unique label sets i.e all the chunks with same labels would be stored in a group | ||
| // chunk details are stored in groups for each unique label set to avoid storing them repetitively for each chunk | ||
| func groupChunks(chunks []chunk.Chunk, deleteFrom, deleteThrough model.Time) []ChunksGroup { | ||
| func groupChunks(chunks []chunk.Chunk, deleteFrom, deleteThrough model.Time, includedChunkIDs map[string]struct{}) ([]ChunksGroup, map[string]struct{}) { |
There was a problem hiding this comment.
Is it more like processedChunkIDs?
Also there is no need to return it back from the function – modifications are already visible to the caller. Function comment should however mention this fact (that passed map is modified).
Signed-off-by: Sandeep Sukhani <[email protected]>
pracucci
approved these changes
Apr 17, 2020
There was a problem hiding this comment.
Thanks @sandeepsukhani ! Let's fix the CHANGELOG and then we can merge it 🎉
CHANGELOG.md
Outdated
Comment on lines
25
to
28
| - `purger_delete_requests_received_total`: Number of delete requests received per user. | ||
| - `purger_delete_requests_processed_total`: Number of delete requests processed per user. | ||
| - `purger_delete_requests_chunks_selected_total`: Number of chunks selected while building delete plans per user. | ||
| - `purger_delete_requests_processing_failures_total`: Number of delete requests processing failures per user. |
There was a problem hiding this comment.
Suggested change
| - `purger_delete_requests_received_total`: Number of delete requests received per user. | |
| - `purger_delete_requests_processed_total`: Number of delete requests processed per user. | |
| - `purger_delete_requests_chunks_selected_total`: Number of chunks selected while building delete plans per user. | |
| - `purger_delete_requests_processing_failures_total`: Number of delete requests processing failures per user. | |
| - `cortex_purger_delete_requests_received_total`: Number of delete requests received per user. | |
| - `cortex_purger_delete_requests_processed_total`: Number of delete requests processed per user. | |
| - `cortex_purger_delete_requests_chunks_selected_total`: Number of chunks selected while building delete plans per user. | |
| - `cortex_purger_delete_requests_processing_failures_total`: Number of delete requests processing failures per user. |
Signed-off-by: Sandeep Sukhani <[email protected]>
sandeepsukhani
force-pushed
the
delete-series-metrics
branch
from
4603c91 to
f1a4dce
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
Adds some metrics for monitoring delete requests. It also eliminates duplicate entries of chunks which overlap multiple plans/days.
purger_delete_requests_received_total: Number of delete requests received per user.purger_delete_requests_processed_total: Number of delete requests processed per user.purger_delete_requests_chunks_selected_total: Number of chunks selected while building delete plans per user.purger_delete_requests_processing_failure: Delete requests processing failure for each user.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]