TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 101 Pull requests 4 Projects Releases 1 Activity Actions

Write cache metrcis #378

Merged
fyrchik merged 4 commits from dstepanov-yadro/frostfs-node:feat/write-cache-metrics into master 2023-05-24 10:18:41 +00:00
Conversation 0 Commits 4 Files changed 19 +514 -69
dstepanov-yadro commented 2023-05-19 08:19:07 +00:00
Member
Copy link

Metrics added:

  • put, get/head, delete duration & counters. Duration implemented with histogram vector metric labeled with shard id, success flag and storage type. Counters implemented with counter vectors labeled with shard id, success flag and storage type

  • flush and evict counters labeled with shard id, storage type and success flag (for flush only)

  • actual object count in write cache labeled with shard id and storage type

  • estimated cache size (object count x max allowed object size). Estimated size used by put objects.

  • writecache mode metric.

Metrics added: - put, get/head, delete duration & counters. Duration implemented with histogram vector metric labeled with shard id, success flag and storage type. Counters implemented with counter vectors labeled with shard id, success flag and storage type - flush and evict counters labeled with shard id, storage type and success flag (for flush only) - actual object count in write cache labeled with shard id and storage type - estimated cache size (object count x max allowed object size). Estimated size used by put objects. - writecache mode metric.
dstepanov-yadro force-pushed feat/write-cache-metrics from 91f6a694e9 to 914a37b99c 2023-05-19 09:57:56 +00:00 Compare
dstepanov-yadro force-pushed feat/write-cache-metrics from 914a37b99c to 8a7ffacf4b 2023-05-19 10:47:06 +00:00 Compare
dstepanov-yadro force-pushed feat/write-cache-metrics from 8a7ffacf4b to 4f6e05e208 2023-05-19 11:07:32 +00:00 Compare
dstepanov-yadro force-pushed feat/write-cache-metrics from 4f6e05e208 to 9aaaf9a367 2023-05-19 15:23:02 +00:00 Compare
dstepanov-yadro force-pushed feat/write-cache-metrics from 9aaaf9a367 to 38f95af11d 2023-05-19 15:51:53 +00:00 Compare
dstepanov-yadro changed title from WIP: Write cache metrcis to Write cache metrcis 2023-05-22 08:03:51 +00:00
dstepanov-yadro requested review from storage-core-committers 2023-05-22 08:04:01 +00:00
dstepanov-yadro requested review from storage-core-developers 2023-05-22 08:04:01 +00:00
acid-ant approved these changes 2023-05-22 08:26:34 +00:00
aarifullin approved these changes 2023-05-22 10:34:11 +00:00
aarifullin left a comment
Member
Copy link

LGTM

LGTM
ale64bit reviewed 2023-05-23 08:27:02 +00:00
pkg/local_object_storage/writecache/metrics.go Outdated
@ -0,0 +32,4 @@
type metricsStub struct{}
func (s *metricsStub) Get(_ time.Duration, _ bool, _ StorageType) {}
ale64bit commented 2023-05-23 08:23:24 +00:00
Member
Copy link

you can just delete all placeholders, since none is used, e.g.

func (s *metricsStub) Get(time.Duration, bool, StorageType) {}

Same elsewhere.

you can just delete all placeholders, since none is used, e.g. ``` func (s *metricsStub) Get(time.Duration, bool, StorageType) {} ``` Same elsewhere.
dstepanov-yadro commented 2023-05-23 09:37:25 +00:00
Author
Member
Copy link

fixed

fixed
ale64bit marked this conversation as resolved
pkg/local_object_storage/writecache/options.go Outdated
@ -166,1 +168,4 @@
}
// WithMetrics sets metrics implementation.
func WithMetrics(metrics metrics) Option {
ale64bit commented 2023-05-23 08:12:12 +00:00
Member
Copy link

this interface should be exported if it's used as part of the public API of this package?

this interface should be exported if it's used as part of the public API of this package?
dstepanov-yadro commented 2023-05-23 09:40:04 +00:00
Author
Member
Copy link

Fixed. I don't think someone will mock it:)

Fixed. I don't think someone will mock it:)
ale64bit marked this conversation as resolved
pkg/local_object_storage/writecache/put.go Outdated
@ -34,2 +35,4 @@
defer span.End()
startedAt := time.Now()
added := false
ale64bit commented 2023-05-23 08:17:00 +00:00
Member
Copy link

hmm, is it really better having these two vars assigned than just calling the c.metrics.Put where they are assigned? Following the flow seems a bit messier this way, IMHO.

hmm, is it really better having these two vars assigned than just calling the `c.metrics.Put` where they are assigned? Following the flow seems a bit messier this way, IMHO.
dstepanov-yadro commented 2023-05-23 09:46:55 +00:00
Author
Member
Copy link

It seems to me that it's easier this way: negative case by default, but for success way we need to change vars.

It seems to me that it's easier this way: negative case by default, but for success way we need to change vars.
👍 1
ale64bit marked this conversation as resolved
pkg/metrics/node.go Outdated
@ -33,12 +34,16 @@ func NewNodeMetrics() *NodeMetrics {
})
mustRegister(epoch)
wcMetrcis := newWCMetrics()
ale64bit commented 2023-05-23 08:18:48 +00:00
Member
Copy link

s/wcMetrcis/wcMetrics

s/`wcMetrcis`/`wcMetrics`
dstepanov-yadro commented 2023-05-23 09:40:58 +00:00
Author
Member
Copy link

fixed

fixed
ale64bit marked this conversation as resolved
pkg/metrics/writecache.go Outdated
@ -0,0 +39,4 @@
modeMetrics map[shardIDMode]metric[prometheus.GaugeFunc]
modeValues map[string]string
modeMtx *sync.RWMutex
ale64bit commented 2023-05-23 08:19:28 +00:00
Member
Copy link

no need for pointer

no need for pointer
dstepanov-yadro commented 2023-05-23 09:43:09 +00:00
Author
Member
Copy link

fixed

fixed
ale64bit marked this conversation as resolved
pkg/metrics/writecache.go Outdated
@ -0,0 +177,4 @@
mustRegister(m.evictCounter)
}
func setWCDuration(m *metric[*prometheus.HistogramVec], shardID string, success bool, d time.Duration) {
ale64bit commented 2023-05-23 08:26:23 +00:00
Member
Copy link

I'm not sure, but do we actually need to pass these (i.e. m) using pointers?

I'm not sure, but do we actually need to pass these (i.e. `m`) using pointers?
dstepanov-yadro commented 2023-05-23 09:58:48 +00:00
Author
Member
Copy link

replaced with value pointer

replaced with value pointer
ale64bit marked this conversation as resolved
dstepanov-yadro force-pushed feat/write-cache-metrics from daf63fad63 to d067817895 2023-05-23 09:37:22 +00:00 Compare
dstepanov-yadro force-pushed feat/write-cache-metrics from d067817895 to 9366e26de3 2023-05-23 09:39:45 +00:00 Compare
dstepanov-yadro force-pushed feat/write-cache-metrics from 9366e26de3 to ce714c9c89 2023-05-23 09:40:56 +00:00 Compare
dstepanov-yadro force-pushed feat/write-cache-metrics from ce714c9c89 to 180c33a101 2023-05-23 09:43:07 +00:00 Compare
dstepanov-yadro force-pushed feat/write-cache-metrics from 180c33a101 to f3763c1f40 2023-05-23 09:58:35 +00:00 Compare
ale64bit approved these changes 2023-05-23 10:01:31 +00:00
ale64bit reviewed 2023-05-23 10:07:00 +00:00
pkg/metrics/node.go Outdated
@ -33,12 +34,16 @@ func NewNodeMetrics() *NodeMetrics {
})
mustRegister(epoch)
wcMetrics := newWCMetrics()
ale64bit commented 2023-05-23 10:07:00 +00:00
Member
Copy link

I would personally avoid this sort of abbreviation in type names. writeCacheMetrics is not that long, and autocomplete helps.

I would personally avoid this sort of abbreviation in type names. `writeCacheMetrics` is not that long, and autocomplete helps.
dstepanov-yadro commented 2023-05-23 11:28:03 +00:00
Author
Member
Copy link

fixed

fixed
ale64bit marked this conversation as resolved
ale64bit reviewed 2023-05-23 10:09:33 +00:00
pkg/metrics/writecache.go Outdated
@ -0,0 +60,4 @@
}
}
func (m *wcMetrics) WriteCacheGetDurationAdd(shardID string, success bool, d time.Duration) {
ale64bit commented 2023-05-23 10:09:33 +00:00
Member
Copy link

eventually we will probably want to think about better structuring this (not specific to this PR). I suspect namespacing the metrics with prefixes in method names won't scale :))

eventually we will probably want to think about better structuring this (not specific to this PR). I suspect namespacing the metrics with prefixes in method names won't scale :))
fyrchik commented 2023-05-23 10:21:29 +00:00
Owner
Copy link

Maybe we can try it here already? Like nodeMetrics.WriteCache()

Maybe we can try it here already? Like `nodeMetrics.WriteCache()`
ale64bit commented 2023-05-23 10:23:21 +00:00
Member
Copy link

if @dstepanov-yadro doesn't mind, that would be ideal.

if @dstepanov-yadro doesn't mind, that would be ideal.
dstepanov-yadro commented 2023-05-23 11:28:13 +00:00
Author
Member
Copy link

done

done
ale64bit marked this conversation as resolved
dstepanov-yadro force-pushed feat/write-cache-metrics from f3763c1f40 to 70fa1403f2 2023-05-23 11:28:01 +00:00 Compare
fyrchik reviewed 2023-05-23 13:26:57 +00:00
pkg/local_object_storage/engine/metrics.go Outdated
@ -27,0 +29,4 @@
}
type WriteCacheMetrics interface {
WriteCacheGetDurationAdd(shardID string, success bool, d time.Duration)
fyrchik commented 2023-05-22 17:01:08 +00:00
Owner
Copy link

We already have AddXDuration pattern for names in this package, why not follow?

We already have `AddXDuration` pattern for names in this package, why not follow?
dstepanov-yadro commented 2023-05-23 13:52:59 +00:00
Author
Member
Copy link

fixed

fixed
fyrchik marked this conversation as resolved
pkg/local_object_storage/writecache/flush.go Outdated
@ -241,2 +241,3 @@
// flushObject is used to write object directly to the main storage.
func (c *cache) flushObject(ctx context.Context, obj *object.Object, data []byte) error {
func (c *cache) flushObject(ctx context.Context, obj *object.Object, data []byte, st StorageType) error {
success := true
fyrchik commented 2023-05-23 13:24:16 +00:00
Owner
Copy link

What about declaring err here instead?

What about declaring `err` here instead?
dstepanov-yadro commented 2023-05-23 13:42:25 +00:00
Author
Member
Copy link

done

done
fyrchik marked this conversation as resolved
pkg/local_object_storage/writecache/metrics.go Outdated
@ -0,0 +13,4 @@
}
const (
StorageTypeUndefined StorageType = ""
fyrchik commented 2023-05-23 11:56:41 +00:00
Owner
Copy link

Does it have any specific meaning for the person who views metrics?

Does it have any specific meaning for the person who views metrics?
dstepanov-yadro commented 2023-05-23 13:39:32 +00:00
Author
Member
Copy link

This value is used when success = false, so replaced with null value

This value is used when success = false, so replaced with `null` value
fyrchik commented 2023-05-24 07:53:01 +00:00
Owner
Copy link

Hm, so we can have failed counter increasing both for "any fail" and "fail specific to some storage type"?

Hm, so we can have failed counter increasing both for "any fail" and "fail specific to some storage type"?
fyrchik commented 2023-05-24 07:56:00 +00:00
Owner
Copy link

I mean may be it's better to have 1 total for everything and specific counters which could increase together with the total one?

I mean may be it's better to have 1 total for everything and specific counters which _could_ increase together with the total one?
dstepanov-yadro commented 2023-05-24 07:59:17 +00:00
Author
Member
Copy link

Why it is better?
Now this case can be resolved with selector {success=false}.

Why it is better? Now this case can be resolved with selector {success=false}.
fyrchik marked this conversation as resolved
pkg/local_object_storage/writecache/metrics.go Outdated
@ -0,0 +14,4 @@
const (
StorageTypeUndefined StorageType = ""
StorageTypeDB StorageType = "db"
fyrchik commented 2023-05-23 11:56:20 +00:00
Owner
Copy link

Why are they exported?

Why are they exported?
dstepanov-yadro commented 2023-05-23 13:40:21 +00:00
Author
Member
Copy link

they are used in engine pkg

they are used in `engine` pkg
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed feat/write-cache-metrics from 70fa1403f2 to b438f55e7d 2023-05-23 13:39:18 +00:00 Compare
dstepanov-yadro force-pushed feat/write-cache-metrics from b438f55e7d to a3ecf15929 2023-05-23 13:42:22 +00:00 Compare
dstepanov-yadro force-pushed feat/write-cache-metrics from a3ecf15929 to 639168b74f 2023-05-23 13:52:37 +00:00 Compare
fyrchik approved these changes 2023-05-24 07:56:17 +00:00
dstepanov-yadro force-pushed feat/write-cache-metrics from 639168b74f to c81d44a06d 2023-05-24 08:25:21 +00:00 Compare
fyrchik merged commit a1823423cf into master 2023-05-24 10:18:41 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
acid-ant
aarifullin
ale64bit
fyrchik
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#378
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:feat/write-cache-metrics"

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?