TrueCloudLab
/
frostfs-node
18
1
Fork 20
Code Issues 88 Pull Requests 8 Actions Releases Activity

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
dstepanov-yadro commented 2023-05-19 08:19:07 +00:00
Collaborator

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
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

s/wcMetrcis/wcMetrics

s/`wcMetrcis`/`wcMetrics`
dstepanov-yadro commented 2023-05-23 09:40:58 +00:00
Poster
Collaborator

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
Collaborator

no need for pointer

no need for pointer
dstepanov-yadro commented 2023-05-23 09:43:09 +00:00
Poster
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

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

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
Collaborator

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
Poster
Collaborator

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

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
Poster
Collaborator

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

What about declaring err here instead?

What about declaring `err` here instead?
dstepanov-yadro commented 2023-05-23 13:42:25 +00:00
Poster
Collaborator

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

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
Poster
Collaborator

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

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

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
Poster
Collaborator

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

Why are they exported?

Why are they exported?
dstepanov-yadro commented 2023-05-23 13:40:21 +00:00
Poster
Collaborator

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 Label
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
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
There is no content yet.
Delete Branch "dstepanov-yadro/frostfs-node:feat/write-cache-metrics"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?