TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 106 Pull requests 12 Projects Releases 2 Activity Actions 2

Use labels for metrics #444

Merged
fyrchik merged 1 commit 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 +2 -2
dstepanov-yadro commented 2023-06-13 12:07:31 +00:00
Member
Copy link

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
Member
Copy link

Ca we use strconv.FormatBool() here?

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

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
Copy link

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
Author
Member
Copy link

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
Copy link

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
Author
Member
Copy link

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
Copy link

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
Author
Member
Copy link

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
Copy link

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
Author
Member
Copy link

For consistency with other code.

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

With what code?

With what code?
fyrchik marked this conversation as resolved
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
[#424] metrics: Rename mode to mode_info
All checks were successful
ci/woodpecker/pr/pre-commit Pipeline was successful
Details
ci/woodpecker/push/pre-commit Pipeline was successful
Details
69b788a90b 
To follow best practice.

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

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
Copy link

@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
Copy link

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
Copy link

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
Author
Member
Copy link

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
Author
Member
Copy link

@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 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
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
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:fix/wc_metrics_labels"

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?