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

Use labels for metrics #444

Merged
fyrchik merged 1 commits from dstepanov-yadro/frostfs-node:fix/wc_metrics_labels into master 2023-07-26 21:07:59 +00:00
Conversation 5 Commits 1 Files Changed 2
dstepanov-yadro commented 2023-06-13 12:07:31 +00:00
Collaborator

Closes #424
Closes #432

Closes #424 Closes #432 * The names of the metrics have been changed in accordance with https://prometheus.io/docs/practices/naming/ * Added use of labels for the same metrics * Embedding replaced with fields
dstepanov-yadro changed title from Use labels for metrics to WIP: Use labels for metrics 2023-06-13 12:07:40 +00:00
dstepanov-yadro force-pushed fix/wc_metrics_labels from 13621abe10 to d11c0cb878 2023-06-13 13:58:40 +00:00 Compare
dstepanov-yadro force-pushed fix/wc_metrics_labels from d11c0cb878 to 321dd2e464 2023-06-13 16:09:50 +00:00 Compare
dstepanov-yadro changed title from WIP: Use labels for metrics to Use labels for metrics 2023-06-14 08:12:02 +00:00
acid-ant approved these changes 2023-06-14 08:35:29 +00:00
pkg/metrics/types.go Outdated
@ -0,0 +11,4 @@
if !v.Valid {
return ""
}
return fmt.Sprintf("%v", v.Bool)
acid-ant commented 2023-06-14 08:31:09 +00:00
Collaborator

Ca we use strconv.FormatBool() here?

Ca we use `strconv.FormatBool()` here?
dstepanov-yadro commented 2023-06-14 09:35:07 +00:00
Poster
Collaborator

fixed

fixed
acid-ant marked this conversation as resolved
dstepanov-yadro force-pushed fix/wc_metrics_labels from eb944e1a4a to 540dd8a7e0 2023-06-14 09:15:46 +00:00 Compare
fyrchik commented 2023-06-14 11:15:05 +00:00
Owner

Please, resolve conflicts.

Please, resolve conflicts.
dstepanov-yadro force-pushed fix/wc_metrics_labels from 540dd8a7e0 to 2a8e05e070 2023-06-14 12:04:42 +00:00 Compare
dstepanov-yadro commented 2023-06-14 12:04:55 +00:00
Poster
Collaborator

Please, resolve conflicts.

Done

> Please, resolve conflicts. Done
fyrchik approved these changes 2023-06-14 14:37:27 +00:00
pkg/local_object_storage/engine/writecache.go
@ -64,3 +64,3 @@
func (m *writeCacheMetrics) Get(d time.Duration, success bool, st writecache.StorageType) {
m.metrics.AddGetDuration(m.shardID, success, d, st.String())
m.metrics.AddMethodDuration(m.shardID, "Get", success, d, st.String())
fyrchik commented 2023-06-14 13:29:02 +00:00
Owner

Why Get and not get or GET? (just a question)

Why `Get` and not `get` or `GET`? (just a question)
dstepanov-yadro commented 2023-06-14 15:37:04 +00:00
Poster
Collaborator

Because it is just method name with capital letter

Because it is just method name with capital letter
fyrchik marked this conversation as resolved
pkg/metrics/mode.go Outdated
@ -0,0 +30,4 @@
}
}
func (m *shardIDModeValue) SetMode(shardID string, mode string) {
fyrchik commented 2023-06-14 14:34:28 +00:00
Owner

So we have const shardID and mode labels and create a new metric for each.
Why not just have variable labels?
Also IMO readonly{shard="abc"} + degraded{shard="abc"} is enough -- this is what we are interested in, so instead of 4 metrics for each shard we have 2.

So we have const `shardID` and `mode` labels and create a new metric for each. Why not just have variable labels? Also IMO `readonly{shard="abc"}` + `degraded{shard="abc"}` is enough -- this is what we are interested in, so instead of 4 metrics for each shard we have 2.
dstepanov-yadro commented 2023-06-14 15:35:30 +00:00
Poster
Collaborator

Why not just have variable labels?

Agree, fixed

Also IMO readonly{shard="abc"} + degraded{shard="abc"} is enough

So we have 2 metrics instead of 1, because we have label "mode" with string value

> Why not just have variable labels? Agree, fixed > Also IMO readonly{shard="abc"} + degraded{shard="abc"} is enough So we have 2 metrics instead of 1, because we have label "mode" with string value
fyrchik marked this conversation as resolved
pkg/metrics/node.go
@ -17,3 +11,1 @@
treeService *treeServiceMetrics
epoch prometheus.Gauge
gc *gcMetrics
engine *engineMetrics
fyrchik commented 2023-06-14 14:37:13 +00:00
Owner

I think I get why you do not like embedding, by why adding a pointer here?

I think I get why you do not like embedding, by why adding a pointer here?
dstepanov-yadro commented 2023-06-14 15:38:40 +00:00
Poster
Collaborator

For consistency with other code.

For consistency with other code.
fyrchik commented 2023-06-15 11:49:03 +00:00
Owner

With what code?

With what code?
fyrchik marked this conversation as resolved
fyrchik requested review from fyrchik 2023-06-14 14:37:55 +00:00
dstepanov-yadro force-pushed fix/wc_metrics_labels from 2a8e05e070 to 26b305f82b 2023-06-14 15:33:14 +00:00 Compare
dstepanov-yadro added 1 commit 2023-06-14 15:41:07 +00:00
ci/woodpecker/pr/pre-commit Pipeline was successful Details
ci/woodpecker/push/pre-commit Pipeline was successful Details
69b788a90b [#424] metrics: Rename mode to mode_info 
To follow best practice.

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
dstepanov-yadro commented 2023-06-14 15:41:54 +00:00
Poster
Collaborator

Also renamed mode -> mode_info because it is pseudo-metric: https://prometheus.io/docs/practices/naming/

Also renamed `mode` -> `mode_info` because it is pseudo-metric: https://prometheus.io/docs/practices/naming/
fyrchik commented 2023-06-15 11:50:34 +00:00
Owner

@dstepanov-yadro it is said that pseudo-metric provides info about the binary, shard mode is a valid runtime state, it is not about the binary. Am I missing something here?

@dstepanov-yadro it is said that pseudo-metric provides info about the _binary_, shard mode is a valid runtime state, it is not about the binary. Am I missing something here?
fyrchik reviewed 2023-06-15 11:54:45 +00:00
pkg/metrics/mode.go
@ -0,0 +22,4 @@
}
func (m *shardIDModeValue) SetMode(shardID string, mode string) {
m.modeValue.DeletePartialMatch(prometheus.Labels{
fyrchik commented 2023-06-15 11:54:45 +00:00
Owner

So in theory we can receive no metrics for an existing shard here?
Probably not very important, but may be this is "easily" fixable?

So in theory we _can_ receive no metrics for an existing shard here? Probably not very important, but may be this is "easily" fixable?
fyrchik commented 2023-06-15 12:01:37 +00:00
Owner

Like, receiving 2 may be better -- we are transitioning between states anyway.

Like, receiving 2 may be better -- we are transitioning between states anyway.
dstepanov-yadro commented 2023-06-15 14:05:42 +00:00
Poster
Collaborator

Yes, we can receive no metric for shard for some period of time when mode changes. We can use GaugeFunc to get continous graph.
But i think it is the limitation of Go client for Prometheus, because GaugeFunc doesn't support removing, so deleted shard will be presented until restart.

Yes, we can receive no metric for shard for some period of time when mode changes. We can use `GaugeFunc` to get continous graph. But i think it is the limitation of Go client for Prometheus, because `GaugeFunc` doesn't support removing, so deleted shard will be presented until restart.
fyrchik approved these changes 2023-06-15 11:54:48 +00:00
dstepanov-yadro commented 2023-06-15 14:01:12 +00:00
Poster
Collaborator

@dstepanov-yadro it is said that pseudo-metric provides info about the binary, shard mode is a valid runtime state, it is not about the binary. Am I missing something here?

Agree, but i think we can extend this point to mode value. I think it will be clever use text label value instead of some magic number.

> @dstepanov-yadro it is said that pseudo-metric provides info about the _binary_, shard mode is a valid runtime state, it is not about the binary. Am I missing something here? Agree, but i think we can extend this point to mode value. I think it will be clever use text label value instead of some magic number.
fyrchik merged commit 69b788a90b into master 2023-06-15 14:10:28 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
acid-ant
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
3 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#444
There is no content yet.
Delete Branch "dstepanov-yadro/frostfs-node:fix/wc_metrics_labels"

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?