Support Block Adapter metrics with adapter_stats_id #8680
Conversation
itaigilo
added
exclude-changelog
E2E Test Results - DynamoDB Local - Local Block Adapter |
E2E Test Results - Quickstart |
itaigilo
changed the title
itaigilo
requested review from
N-o-Z,
nadavsteindler and
guy-har
and removed request for
a team
|
Not sure what I'm meant to review here: PR says "stats", code says "metrics". I think we need only to rename the PR - if it's about stats then we might need to do rather different things. |
Well, let's have it written: If so, not the mix, since the Having said that, I don't mind renaming this PR 😄 |
itaigilo
changed the title
| adapterStatsID *string | ||
| } | ||
|
|
||
| func BuildHistogramsInstance(name string, adapterStatsID *string) Histograms { |
There was a problem hiding this comment.
What is adapterStatsID? not sure what this means, please change name or document
| func BuildHistogramsInstance(name string, adapterStatsID *string) Histograms { | ||
| labelNames := []string{"operation", "error"} | ||
| if adapterStatsID != nil { | ||
| labelNames = append(labelNames, "adapter_stats_id") |
There was a problem hiding this comment.
Same for the label, I have no idea what this represents
| if s.adapterStatsID != nil { | ||
| labels = append(labels, *s.adapterStatsID) | ||
| } |
There was a problem hiding this comment.
Not a blocker (thinking out load), but this one disturbs me a bit, why do we need to check every report? It's a bit weird that we have a label that is added only in some cases.
Maybe having two instead of making it generic, or providing getDurationHistogramWithLables. Also maybe moving the optional to the beginning will be safer because we tend to add labels to the end.
IMO just add the label, for everyone, it shouldn't affect too much if it has only one value
| durationHistograms.WithLabelValues(operation, isErrStr).Observe(time.Since(start).Seconds()) | ||
| if sizeBytes != nil { | ||
| requestSizeHistograms.WithLabelValues(operation, isErrStr).Observe(float64(*sizeBytes)) | ||
| func NewS3Stats(adapterStatsID *string) block.Histograms { |
There was a problem hiding this comment.
| func NewS3Stats(adapterStatsID *string) block.Histograms { | |
| func NewStats(adapterStatsID *string) block.Histograms { |
| durationHistograms.WithLabelValues(operation, isErrStr).Observe(time.Since(start).Seconds()) | ||
| if sizeBytes != nil { | ||
| requestSizeHistograms.WithLabelValues(operation, isErrStr).Observe(float64(*sizeBytes)) | ||
| func NewAzureStats(adapterStatsID *string) block.Histograms { |
There was a problem hiding this comment.
| func NewAzureStats(adapterStatsID *string) block.Histograms { | |
| func NewStats(adapterStatsID *string) block.Histograms { |
| var durationHistograms = promauto.NewHistogramVec( | ||
| prometheus.HistogramOpts{ | ||
| Name: name + "_operation_duration_seconds", | ||
| Help: "durations of outgoing " + name + " operations", | ||
| }, | ||
| labelNames, | ||
| ) |
There was a problem hiding this comment.
I believe there is an issue here, each histogram can be initiated one time per name, otherwise it will panic. This means that:
- We can't initiate a block adapter twice!
- Don't think this can be used for 2 different adapterStatesIDs
Closes #8672.
Change Description
Currently, the Block Adapters that use
reportMetricsonly report withoperationandisErrStrlabels -Adding another
adapter_stats_idlabel to it, to allow better tracing.