[#371] ir: Add morph cache metrics #400
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#400
Loading…
Reference in a new issue
No description provided.
Delete branch "aarifullin/frostfs-node:feature/371-morph_cache_metr"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Signed-off-by: Airat Arifullin a.arifullin@yadro.com
[#371] ir: Add morph cache metrics:to WIP: [#371] ir: Add morph cache metrics:bf42a35286
to1bef474d37
@ -0,0 +64,4 @@
var _ MorphCacheMetrics = (*morphCacheMetrics)(nil)
func (m *morphCacheMetrics) IncGetNNSContractHash(success bool) {
m.getGroupKeyCounter.value.With(prometheus.Labels{
I was thinking is it worth it to add the nns contract hash label but after all I concluded that it's not helpful information from the point of metric calculation.
The second point - the events like getting NNS contract hash and group key are rare compared to getting tx height but I've added them anyway
WIP: [#371] ir: Add morph cache metrics:to [#371] ir: Add morph cache metrics:@ -0,0 +32,4 @@
type morphCacheMetrics struct {
getNNSContractHashCounter metric[*prometheus.CounterVec]
getNNSContractHashDuration metric[*prometheus.HistogramVec]
According to this #395 - is it necessary for counter here too?
Yeah, I think so. These will be removed
I checked out @dstepanov-yadro 's PRs and his implementation for
writeCache
.You can look at these lines and this
writeCache
' feature - you can see that cache usesdb
andfsTree
.I need to separate flags -
cache_hit
andfound
, because we can encounter these situations:If I use only
success
flag like this, then we will never know was it cache miss or error during processing cache miss@dstepanov-yadro , please, fix me, if I am not correct
I think than @acid-ant suggests using
getNNSContractHashDuration metric[*prometheus.HistogramVec]
with two headers: cache_hit and success.I've introduced
cache_hit = true/false
andfound = true/false
. I find thesuccess
label inconvenient because it's not verbose from my POV :)P.S. Replaced it to
success
anyway[#371] ir: Add morph cache metrics:to WIP: [#371] ir: Add morph cache metrics:1bef474d37
tod3e5057045
WIP: [#371] ir: Add morph cache metrics:to [#371] ir: Add morph cache metrics:d3e5057045
to380fdb3285
[#371] ir: Add morph cache metrics:to [#371] ir: Add morph cache metricsThe task was also about caches in here https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/cmd/frostfs-node/cache.go
Also, we would like to have the same metrics for all caches:
miss duration
metric with this common one, let's discuss. The rationale is that we can have a separate duration metric just for morph client, irregardless of the cache.Having this in mind, maybe we should create some generic structure for such usecase?
We can then provide a name to this cache ("nns_hash", "eacl" etc.) and not reflect it in methods. It seems easier both to read and maintain.
@ -0,0 +14,4 @@
mcGetGroupKey = "get_group_key"
mcGetTxHeight = "get_tx_height"
mcNNSContractID = "nns_contract_id"
mcNNSContractHash = "nns_contract_hash"
Is it used anywhere?
@ -89,2 +90,4 @@
gKey *keys.PublicKey
txHeights *lru.Cache[util.Uint256, uint32]
morphCacheMetrics metrics.MorphCacheMetrics
It resides in
morph/Client.cache
, may be call the field just "metrics"?@ -84,0 +88,4 @@
defer func() {
c.cache.morphCacheMetrics.IncGetNNSContractHash(cacheHit)
c.cache.morphCacheMetrics.AddGetNNSContractHashDuration(found, time.Since(startedAt))
The comment says
// Duration of processing get nns contract hash request if cache miss happens
, but this line is executed unconditionally. Whom to believe?@ -221,7 +233,15 @@ func (c *Client) SetGroupSignerScope() error {
// contractGroupKey returns public key designating FrostFS contract group.
func (c *Client) contractGroupKey() (*keys.PublicKey, error) {
var cacheHit, found bool
We use this
found
as a success indicator, what aboutsuccess
?Alright. I give up :) It seems the majority wants to see
success
flag - I'll fix it.For me
success
label doesn't fit well with metric name.Let's consider
get_tx_height
/tx_height
metric. How does it look in the histogram with labelsuccess
:success = true
because cache has hit OR because cache has missed and we processed this situation and successfullyfound
the requested?success = false
because cache has missed OR because cache has missed and we processed this situation and unsuccessfullyfound
the requested?These two points were in my mind...
P.S. Okay. Agreed. The same can bee applied for
found
labelLet's look at the other side. This is a metric for the cache. I think it's not particularly important here whether we found the object in the end or not. So
found
ismorph client
metric, notmorph cache
.@fyrchik @aarifullin what do you think?
From my POV - it's better to keep
morph cache
because we count hits/misses and the duration of the miss processingI've posted above the difference between metrics for
morphCache
andwriteCache
. So, we can make "some generic structure" only for "in-memory" cases:380fdb3285
tofbb6252ba8
[#371] ir: Add morph cache metricsto WIP: [#371] ir: Add morph cache metricsfbb6252ba8
to0aa32cc936
@ -9,12 +9,20 @@ import (
const treeServiceLabelSuccess = "success"
type TreeMetricsRegister interface {
tree.MetricsRegister
has been moved frompkg/services/tree/metrics.go
to here and renamed because this caused import cycleWIP: [#371] ir: Add morph cache metricsto [#371] ir: Add morph cache metricsI have given more detailed explanation in this post
[#371] ir: Add morph cache metricsto WIP: [#371] ir: Add morph cache metrics0aa32cc936
to050ff1d56d
WIP: [#371] ir: Add morph cache metricsto [#371] ir: Add morph cache metrics@ -0,0 +58,4 @@
Name: "tx_height",
Help: "Transaction height in the morph cache.",
},
[]string{mcTxHash, mcSuccess}),
Is mcTxHash countable? If not we will get a lot of metric records.
Alright, probably I misunderstood the meaing of the labels. I've fixed!
050ff1d56d
toa05637712e
@ -0,0 +40,4 @@
nnsContractHashDuration: metrics.NewHistogramVec(prometheus.HistogramOpts{
Namespace: namespace,
Subsystem: mcSubsystem,
Name: "nns_contract_hash",
See https://prometheus.io/docs/practices/naming/
Oh, sorry... Fixed!
a05637712e
to1e8e5296fb