Support Block Adapter metrics with adapter_stats_id #8680

itaigilo · 2025-02-17T17:36:35Z

Closes # 8672.

Change Description

Currently, the Block Adapters that use reportMetrics only report with operation and isErrStr labels -
Adding another adapter_stats_id label to it, to allow better tracing.

github-actions · 2025-02-17T17:45:33Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2025-02-17T17:46:45Z

E2E Test Results - Quickstart

arielshaqed · 2025-02-24T09:54:26Z

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.

itaigilo · 2025-02-24T11:31:50Z

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:
When you write stats, you mean the pkg/stats package?
And when you write metrics, you mean everything that's grafana/prometheus related?

If so, not the mix, since the stats.go files are handling prometheus stuff...

Having said that, I don't mind renaming this PR 😄

guy-har

Looks good, requesting changes mostly because of the label name, that I'm not sure what it means

guy-har · 2025-02-24T12:15:07Z

pkg/block/metrics.go

+	adapterStatsID        *string
+}
+
+func BuildHistogramsInstance(name string, adapterStatsID *string) Histograms {


What is adapterStatsID? not sure what this means, please change name or document

guy-har · 2025-02-24T12:17:57Z

pkg/block/metrics.go

+func BuildHistogramsInstance(name string, adapterStatsID *string) Histograms {
+	labelNames := []string{"operation", "error"}
+	if adapterStatsID != nil {
+		labelNames = append(labelNames, "adapter_stats_id")


Same for the label, I have no idea what this represents

guy-har · 2025-02-24T12:30:21Z

pkg/block/metrics.go

+	if s.adapterStatsID != nil {
+		labels = append(labels, *s.adapterStatsID)
+	}


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

guy-har · 2025-02-24T12:33:39Z

pkg/block/s3/stats.go

-	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 {


Suggested change

func NewS3Stats(adapterStatsID *string) block.Histograms {

func NewStats(adapterStatsID *string) block.Histograms {

guy-har · 2025-02-24T13:14:11Z

pkg/block/azure/stats.go

-	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 {


Suggested change

func NewAzureStats(adapterStatsID *string) block.Histograms {

func NewStats(adapterStatsID *string) block.Histograms {

guy-har

Requesting changes again but this one is critical.
We can't initiate the same adapter twice, which raises the question, how was this tested? Did we try this out for two Adapters with same name and different AdapterStatID?

guy-har · 2025-02-24T13:48:00Z

pkg/block/metrics.go

+	var durationHistograms = promauto.NewHistogramVec(
+		prometheus.HistogramOpts{
+			Name: name + "_operation_duration_seconds",
+			Help: "durations of outgoing " + name + " operations",
+		},
+		labelNames,
+	)


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

@itaigilo how was this addressed?

OK, I see there's a nil check to ensure initialization happens only once. However, I still believe we should prioritize registering the metric during package-level initialization rather than run time

itaigilo · 2025-02-28T13:20:21Z

I've done a major refactoring here -
Histograms are only created once per name (which is per blockstore type).
Added comments about the limitations there.

Also renamed stats to metrics for clarity.

Tested it with two (S3) blockstores, and it reports each with a different label.

@guy-har PTAL again -
If it's still unclear, let's talk about it F2F.

arielshaqed

Thanks! Guy's point is obviously blocking here.

arielshaqed · 2025-03-05T07:09:48Z

pkg/block/metrics.go

@@ -134,3 +137,59 @@ func (m *MetricsAdapter) GetRegion(ctx context.Context, storageID, storageNamesp
 func (m *MetricsAdapter) RuntimeStats() map[string]string {
 	return m.adapter.RuntimeStats()
 }
+
+type Histograms struct {


Consider calling this a Histogram - after all that's literally the name Prometheus gives it.

arielshaqed · 2025-03-05T07:11:06Z

pkg/block/metrics.go

+		prometheus.HistogramOpts{
+			Name:    name + "_operation_size_bytes",
+			Help:    "handled sizes of outgoing " + name + " operations",
+			Buckets: prometheus.ExponentialBuckets(1, 10, 10), //nolint: mnd


TIL these the buckets we have. That's fairly nasty... (No action in this PR, of course.)

guy-har

I prefer not to initiate metrics on New adapter. I would recommend:

leave the metrics as they were
add the adapter id label
initiated the adapter with a property of adapter id
call reportMetrics with adapterID
I believe this will reduce complexity

guy-har · 2025-03-09T08:59:35Z

pkg/block/metrics.go

+	var durationHistograms = promauto.NewHistogramVec(
+		prometheus.HistogramOpts{
+			Name: name + "_operation_duration_seconds",
+			Help: "durations of outgoing " + name + " operations",
+		},
+		labelNames,
+	)


@itaigilo how was this addressed?

guy-har · 2025-03-09T09:12:36Z

pkg/block/factory/build.go

@@ -132,7 +132,7 @@ func BuildGSClient(ctx context.Context, params params.GS) (*storage.Client, erro
 	return storage.NewClient(ctx, opts...)
 }

-func buildGSAdapter(ctx context.Context, params params.GS) (*gs.Adapter, error) {
+func buildGSAdapter(ctx context.Context, params params.GS, metricsID *string) (*gs.Adapter, error) {


What is metricsID, Isn't it the adapter ID?

itaigilo · 2025-03-10T15:49:20Z

After discussing with @guy-har F2F,
Replaced this PR with this one.

Closing.

Support Block Adapter stats tag

dd9cdfb

itaigilo added exclude-changelog PR description should not be included in next release changelog msb labels Feb 17, 2025

itaigilo added 2 commits February 17, 2025 21:52

Add to S3 and GS adapters

a52fdcd

Rename param

cf2a0bf

itaigilo marked this pull request as ready for review February 17, 2025 20:27

Rename param

8829e03

itaigilo changed the title ~~Support Block Adapter stats tag~~ Support Block Adapter stats blockstoreTag Feb 18, 2025

itaigilo requested review from arielshaqed and a team February 20, 2025 10:49

itaigilo added 2 commits February 21, 2025 19:42

Rename param

1d8e94d

Merge branch 'master' into task/support-block-adapter-stats-tag

d497ff5

itaigilo requested review from N-o-Z, nadavsteindler and guy-har and removed request for a team February 21, 2025 19:51

itaigilo changed the title ~~Support Block Adapter stats blockstoreTag~~ Support Block Adapter metrics with adapter_stats_id Feb 24, 2025

guy-har requested changes Feb 24, 2025

View reviewed changes

itaigilo added 2 commits February 28, 2025 13:09

Refactor

1ecbff0

Merge branch 'master' into task/support-block-adapter-stats-tag

cb6af4f

itaigilo requested a review from guy-har February 28, 2025 13:20

itaigilo added 3 commits February 28, 2025 17:36

Merge branch 'master' into task/support-block-adapter-stats-tag

98f8ce6

Rename param

9482aa6

Merge branch 'master' into task/support-block-adapter-stats-tag

12e8204

arielshaqed reviewed Mar 5, 2025

View reviewed changes

guy-har reviewed Mar 9, 2025

View reviewed changes

itaigilo added the minor-change Used for PRs that don't require issue attached label Mar 10, 2025

itaigilo mentioned this pull request Mar 10, 2025

Add storage_id tag to StorageAdapter metrics #8786

Merged

itaigilo closed this Mar 10, 2025

itaigilo deleted the task/support-block-adapter-stats-tag branch March 10, 2025 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Block Adapter metrics with adapter_stats_id #8680

Support Block Adapter metrics with adapter_stats_id #8680

itaigilo commented Feb 17, 2025 •

edited

Loading

github-actions bot commented Feb 17, 2025

github-actions bot commented Feb 17, 2025

arielshaqed commented Feb 24, 2025

itaigilo commented Feb 24, 2025

guy-har left a comment

guy-har Feb 24, 2025

guy-har Feb 24, 2025

guy-har Feb 24, 2025

guy-har Feb 24, 2025

guy-har Feb 24, 2025

guy-har left a comment

guy-har Feb 24, 2025

guy-har Mar 9, 2025

guy-har Mar 9, 2025

itaigilo commented Feb 28, 2025

arielshaqed left a comment

arielshaqed Mar 5, 2025

arielshaqed Mar 5, 2025

guy-har left a comment

guy-har Mar 9, 2025

guy-har Mar 9, 2025

itaigilo commented Mar 10, 2025

	func NewS3Stats(adapterStatsID *string) block.Histograms {
	func NewStats(adapterStatsID *string) block.Histograms {

	func NewAzureStats(adapterStatsID *string) block.Histograms {
	func NewStats(adapterStatsID *string) block.Histograms {

Support Block Adapter metrics with adapter_stats_id #8680

Support Block Adapter metrics with adapter_stats_id #8680

Conversation

itaigilo commented Feb 17, 2025 • edited Loading

Change Description

github-actions bot commented Feb 17, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Feb 17, 2025

E2E Test Results - Quickstart

arielshaqed commented Feb 24, 2025

itaigilo commented Feb 24, 2025

guy-har left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guy-har left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo commented Feb 28, 2025

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guy-har left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo commented Mar 10, 2025

itaigilo commented Feb 17, 2025 •

edited

Loading