feat: add MetricsProvider interface for pluggable metrics backends by mvanhorn · Pull Request #3481 · IBM/sarama

mvanhorn · 2026-03-22T06:09:05Z

Summary

Introduces a MetricsProvider interface that wraps the current rcrowley/go-metrics usage, providing an incremental path toward replacing the archived dependency without breaking existing users.

New MetricsProvider interface with MetricsCounter, MetricsGauge, MetricsHistogram, and MetricsMeter sub-interfaces matching only the methods sarama actually calls
GoMetricsProvider wrapper that preserves current go-metrics behavior
New optional Config.MetricsProvider field - nil by default, preserving full backward compatibility
Comprehensive test suite including interface contract tests, concurrent access safety, and a stub provider demonstrating custom backend integration

Motivation

rcrowley/go-metrics is archived and unmaintained. The author recommends OpenTelemetry as a replacement. This interface layer enables users to plug in OTel, Prometheus, or any other backend without waiting for a v2 release.

The approach is purely additive - the existing Config.MetricRegistry field and all current behavior is untouched. When MetricsProvider is nil (the default), nothing changes. Internal callsites can be migrated incrementally in follow-up PRs.

Testing

go test -race ./... passes. New tests cover:

GoMetricsProvider wrapper (counter, gauge, histogram, meter, unregister)
Custom stub provider implementing the interface
Config validation with MetricsProvider set
Concurrent access safety with race detector

Addresses #3136

Introduce a MetricsProvider interface that wraps the current rcrowley/go-metrics usage, enabling future backend swaps (OTel, Prometheus) without breaking existing users. The new Config.MetricsProvider field is optional and defaults to nil, preserving the existing MetricRegistry behavior. When set, it takes precedence over MetricRegistry. Includes GoMetricsProvider as the default wrapper implementation and a comprehensive test suite covering the interface contract, the go-metrics wrapper, custom provider implementations, and concurrent access safety. Addresses IBM#3136 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

dnwe

Thanks for proposing this. Looks good overall, I’ve raised a couple of comments.

Currently this isn’t wired up in the metric call sites, were you planning to do that under this PR or with a follow up?

dnwe · 2026-03-22T14:42:50Z

+}
+
+func (p *GoMetricsProvider) NewHistogram(name string) MetricsHistogram {
+	return p.registry.GetOrRegister(name, func() metrics.Histogram {


Not sure why Histogram one is p.registry.GetOrregister when the others are metrics.GetOrRegister which handles the nil case and won’t panic, was that a limitation in go-metrics or just a mistake here?

This is essentially already what it does internally: https://github.com/rcrowley/go-metrics/blob/65e299d6c5c9/histogram.go#L22-L27 Though, it would seem that the original could call into NewExpDecaySample every time… which is probably a bit of a misstep in the go-metrics library.

dnwe · 2026-03-22T14:50:49Z

+}
+
+func (p *GoMetricsProvider) UnregisterAll() {
+	p.registry.UnregisterAll()


I wonder if we want to track which metrics we ourselves added and only unregister those, like we do with cleanupRegistry type today?

mvanhorn · 2026-03-25T01:19:43Z

Thanks for the review, @dnwe.

To answer your question - the call site wiring is planned as a follow-up PR. This one establishes the interface and wrapper so we can migrate incrementally without a big-bang change.

Addressed both inline comments in 5077b34:

Histogram (line 74): go-metrics' GetOrRegisterHistogram requires a Sample parameter, so there's no zero-arg convenience function. The pattern matches sarama's existing getOrRegisterHistogram helper in metrics.go which uses r.GetOrRegister(name, func() ...) for the same reason. Left as-is since it's consistent with the codebase.

UnregisterAll (line 84): Fixed. GoMetricsProvider now tracks which metrics it registered and only unregisters those, matching the cleanupRegistry pattern. Added a test verifying that externally-registered metrics survive UnregisterAll.

puellanivis · 2026-03-28T19:57:02Z

The second commit in the PR has: Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

This is inconsistent with our https://github.com/IBM/sarama/blob/main/CONTRIBUTING.md#ai-assistance contributing guidelines.

PS: According to Thaler v. Perlmutter, under US law, AI cannot be an author in a joint work, because they cannot form legal intent, and this point is central to why they should not be purported as authors, or even as co-authors.

puellanivis · 2026-03-28T20:02:39Z

+type GoMetricsProvider struct {
+	registry metrics.Registry
+	names    map[string]struct{}
+	mu       sync.Mutex


Mutexes should head a paragraph of the values that it guards:

registry metrics.Registry mu sync.Mutex names map[string]struct{}

Addressed in a1d7f8c:

Renamed New{Counter,Gauge,Histogram,Meter} to Get{...} on both interface and implementation

Mutex now heads the guarded-values paragraph

track() uses defer and handles lazy init

UnregisterAll uses delete() in the loop

Added nil-check documentation on Config.MetricsProvider

Re: the Histogram GetOrRegister (line 92) -- agreed it's a quirk of go-metrics. The wrapper matches the existing getOrRegisterHistogram helper in metrics.go so behavior stays consistent.

👍 Yeah, I’m solid behind “I guess this is the necessary workaround.”

puellanivis · 2026-03-28T20:19:12Z

+}
+
+func (p *GoMetricsProvider) NewHistogram(name string) MetricsHistogram {
+	return p.registry.GetOrRegister(name, func() metrics.Histogram {


This is essentially already what it does internally: https://github.com/rcrowley/go-metrics/blob/65e299d6c5c9/histogram.go#L22-L27 Though, it would seem that the original could call into NewExpDecaySample every time… which is probably a bit of a misstep in the go-metrics library.

mvanhorn · 2026-03-29T06:12:24Z

Removed the Co-Authored-By trailer in 00cc9cc. I missed that section of the contributing guide -- won't repeat it.

puellanivis

Just the one point.

puellanivis · 2026-03-30T16:13:04Z

+func NewGoMetricsProvider(registry metrics.Registry) *GoMetricsProvider {
+	return &GoMetricsProvider{
+		registry: registry,
+		names:    make(map[string]struct{}),


Now that we have lazy initialization, we can remove this line.

Removed in 3527aba. Also rebased to add DCO sign-off on all commits.

Verified: go test -race passes — TestGoMetricsProvider* and TestStubMetricsProvider all green. UnregisterAll before any track() call is safe since range over nil map is a no-op in Go.

GoMetricsProvider now only unregisters metrics it created, matching the cleanupRegistry pattern. External metrics registered directly on the registry are preserved. Signed-off-by: Matt Van Horn <mvanhorn@MacBook-Pro.local>

- Rename New{Counter,Gauge,Histogram,Meter} to Get{...} since get-or-create is an implementation detail - Move mutex to head the paragraph of guarded values - Use defer for mutex unlock in track() - Use delete() in UnregisterAll loop instead of map reassignment - Add lazy init in track() for nil-safety - Document nil-check requirement on Config.MetricsProvider Signed-off-by: Matt Van Horn <mvanhorn@MacBook-Pro.local>

track() already handles lazy initialization, so the eager allocation in the constructor is unnecessary. Signed-off-by: Matt Van Horn <mvanhorn@MacBook-Pro.local>

puellanivis

I don’t really see anything wrong with the code anymore, but I also wonder how useful it is that we’ve replaced go-metrics with interfaces that back in go-metrics’s APIs.

That is, this builds in a leverage point to replace go-metrics, but whatever replaces it, is going to have to just contort itself to fit the go-metrics API anyways.

Usually, the recommendation is to build on concrete types, and if we need to upgrade, write a refactor.

Nothing on the author or the design, this is pervasive in CompSci, when there’s only one implementation, it can be difficult to build a true abstraction layer that suits multiple implementations, and not one that just mirrors the only implementation.

But as said, the code is fine.

mvanhorn · 2026-04-05T22:04:53Z

Fair point. The interface does mirror go-metrics closely, and you're right that a replacement backend would still need to map to those semantics. The main value I was targeting is testability - being able to swap in a no-op or recording implementation in tests without importing go-metrics. But I take your point that building a true abstraction ahead of a second implementation risks just mirroring the first.

If you'd rather keep concrete types and refactor when there's a real second backend, I'm happy to simplify. Otherwise the code is ready as-is.

puellanivis · 2026-04-07T13:54:05Z

Agree. I think the code is ready as is. I’ll leave it to the maintainers (rather than me just a collaborator) to make a decision on this, if they’re good with the design decision proposed.

mvanhorn · 2026-04-08T04:11:07Z

Thanks @puellanivis for the thorough review. @dnwe, whenever you get a chance to weigh in on the design approach, happy to adjust if needed.

dnwe · 2026-05-13T12:26:40Z

I spent some more time thinking about this, and I think @puellanivis is correct here - assuming that we'd eventually support (for example) three possible backends of go-metrics, prometheus and opentelemetry, they have fundamentally different models for labels/attributes, meter vs query time rate, lifecycle etc. and if we don't reconcile those at the interface level then its going to be ugly making those work

I think for a pluggable interface we need to make it a purely recording API, not a reading API. So the interface defines how Sarama should report its metrics and in what shape, and then the various adapters will put that into the right native format for each backend. Currently in this PR we have the go-metrics shaped reads encoded.

As part of that change we'd need to make attributes/labels a first-class parameter. Today sarama builds names like "request-rate-for-broker-3-for-topic-foo". That's a workaround for go-metrics having no labels. Prometheus and OTel can't undo that, so end up with high-cardinality metric names instead of label sets.

mvanhorn · 2026-05-13T17:24:04Z

@dnwe You're right. The current interface encodes go-metrics' read API. Translating names like request-rate-for-broker-3-for-topic-foo into Prometheus/OTel labels at the adapter layer is the high-cardinality footgun you'd expect.

Recording-only with first-class labels is a bigger change than this PR allows - every internal callsite touched, names split into (name, attrs) pairs. If that's the direction you want, the cleanest path is closing this and opening an RFC issue to design the interface, then migrating callsites in stages. I can drive that work if it fits sarama's roadmap, or close this and leave the design to whoever picks up #3136 next. Let me know which is useful.

dnwe reviewed Mar 22, 2026

View reviewed changes

puellanivis reviewed Mar 28, 2026

View reviewed changes

mvanhorn force-pushed the osc/feat-metrics-interface branch from 5077b34 to a1d7f8c Compare March 29, 2026 06:12

puellanivis reviewed Mar 30, 2026

View reviewed changes

mvanhorn and others added 3 commits March 30, 2026 12:14

fix: track registered metrics for selective UnregisterAll

eebd8d3

GoMetricsProvider now only unregisters metrics it created, matching the cleanupRegistry pattern. External metrics registered directly on the registry are preserved. Signed-off-by: Matt Van Horn <mvanhorn@MacBook-Pro.local>

refactor: remove redundant map init in NewGoMetricsProvider

3527aba

track() already handles lazy initialization, so the eager allocation in the constructor is unnecessary. Signed-off-by: Matt Van Horn <mvanhorn@MacBook-Pro.local>

mvanhorn force-pushed the osc/feat-metrics-interface branch from 108e72c to 3527aba Compare March 30, 2026 19:14

puellanivis reviewed Apr 5, 2026

View reviewed changes

Uh oh!

Conversation

mvanhorn commented Mar 22, 2026

Summary

Motivation

Testing

Uh oh!

dnwe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvanhorn commented Mar 25, 2026

Uh oh!

puellanivis commented Mar 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvanhorn commented Mar 29, 2026

Uh oh!

puellanivis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

puellanivis left a comment

Choose a reason for hiding this comment

Uh oh!

mvanhorn commented Apr 5, 2026

Uh oh!

puellanivis commented Apr 7, 2026

Uh oh!

mvanhorn commented Apr 8, 2026

Uh oh!

dnwe commented May 13, 2026

Uh oh!

mvanhorn commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

puellanivis commented Mar 28, 2026 •

edited

Loading