feat(sdk): support bring-your-own hasher for metrics via BuildHasher by bryantbiggs · Pull Request #3388 · open-telemetry/opentelemetry-rust

bryantbiggs · 2026-02-24T20:01:01Z

Summary

Following the discussion below, this PR pivots from the original
metrics-use-foldhash feature flag to a bring-your-own-hasher design, as
suggested by @utpilla and @scottgerring.

The metrics hot path (counter.add() / histogram.record()) looks up an
aggregation bucket for each attribute set in an internal HashMap inside
ValueMap. That map is now parameterized over a user-chosen
[BuildHasher], defaulting to the standard library's RandomState
(SipHash-1-3). Users who want more throughput on high-cardinality workloads can
supply their own hasher via the new MeterProviderBuilder::with_hasher:

let provider = SdkMeterProvider::builder()
    .with_hasher(foldhash::fast::RandomState::default())
    .with_reader(reader)
    .build();

No new SDK feature flags, and no new SDK runtime dependencies — users bring
their own hasher crate (foldhash, ahash, or anything implementing
BuildHasher).

Why this approach

This addresses the concerns raised in review:

Safe by default (@cijothomas, @utpilla): the default stays RandomState
(SipHash), which is HashDoS-resistant. The faster, non-resistant path is
strictly opt-in. Existing behavior is unchanged.
No feature-flag / dependency sprawl (@utpilla): rather than adding one
feature flag per hasher (each a new dependency to vet), users pick any hasher.
The foldhash dependency, the metrics-use-foldhash feature, and the
deny.toml exception from the previous revision are gone.
Configurable via the public API + user code (@scottgerring): the choice
lives entirely in user code through with_hasher.

Design / public API

The chosen hasher is monomorphized into the per-instrument ValueMap storage
and then type-erased at the existing Arc<dyn Measure> boundary, via an
internal aggregate-function factory. As a result:

SdkMeterProvider and MeterProviderBuilder stay non-generic —
with_hasher returns the same builder type, and the provider keeps working
through the type-erased dyn MeterProvider path used by
opentelemetry::global. The public API adds exactly one method and no type
parameters; code that does not call with_hasher is entirely unaffected.
The measurement hot path is unchanged: ValueMap is concrete behind the
existing Arc<dyn Measure>, so the only added indirection is a single
dyn dispatch at instrument-creation time (cold path), never on
add/record.
Only std types (BuildHasher, RandomState) appear in the public API, so
no allowed-external-types.toml change is required.

Alternatives considered: exposing the hasher as a public generic type
parameter on SdkMeterProvider<S = RandomState> (threaded through the meter
provider) was prototyped, but it leaks a viral, hard-to-reverse type parameter
into the SDK's most central public type for a niche, set-once performance knob.
The type-erased approach keeps that cost out of the public API.

Benchmark

benches/metrics_hasher.rs compares the counter hot path using the default
SipHash against foldhash installed via with_hasher. Both providers are
identical except for the hash builder. 1600 time series, 4 attributes per
measurement.

Hasher (counter hot path)	Time per `add`	vs. default
`RandomState` (SipHash-1-3, default)	~213 ns	—
`foldhash` via `with_hasher`	~151 ns	−29%

_{AMD Ryzen Threadripper 1900X, x86_64, rustc 1.93.0, criterion 0.5. Single

representative run; absolute numbers are hardware-dependent.}

Run with:

cargo bench --bench metrics_hasher --features metrics,experimental_metrics_custom_reader

Refs: #3371

cijothomas · 2026-02-24T20:06:39Z

ahash is already a transitive dependency via indexmap

We don't use indexmap now. We used to, but removed long ago.

unnecessary here since ValueMap is pub(crate) and keys are not attacker-controlled.

A lot of users provide keys and values from incoming request etc, so we should be safe by default. If a user explicitly opts-in to ahash, then we can do this change.

cijothomas

Thanks for striving to improve metrics perf. As noted in my comments, I am okay with this change if we feature gate it, so users are explicitly opting into this.

@utpilla might have considered/tried this before, but we didn't quite end up adding it - Would want to get his thoughts too.

bryantbiggs · 2026-02-24T21:15:26Z

taking a deeper look - ahash is not as well maintained anymore and there have been better, more modern alternatives that are maintained. its somewhat of a toss up between foldhash and rapidhash; with foldhash performing better with more attributes - I am less familiar with how many attributes are commonly used so I will defer to you all in terms of which one we should add behind a feature flag

codecov · 2026-02-24T22:48:24Z

Codecov Report

❌ Patch coverage is 98.11321% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.9%. Comparing base (2571776) to head (c78a4d6).

Files with missing lines	Patch %	Lines
opentelemetry-sdk/src/metrics/pipeline.rs	93.8%	3 Missing ⚠️
opentelemetry-sdk/src/metrics/mod.rs	96.8%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #3388    +/-   ##
======================================
  Coverage   82.9%   82.9%            
======================================
  Files        130     130            
  Lines      27350   27498   +148     
======================================
+ Hits       22675   22819   +144     
- Misses      4675    4679     +4

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

utpilla · 2026-02-24T23:33:49Z

SipHash's HashDoS resistance is unnecessary here since ValueMap is pub(crate) and keys (Vec<KeyValue>) are not attacker-controlled.

That's not entirely true. While ValueMap is pub(crate), the measurement recording APIs are public, and the SDK simply stores whatever dimensions the calling application provides. If an application passes end-user input directly as metric attributes, those values flow straight into our hash map. Whether that's a realistic threat depends entirely on how the application is instrumented, and as an SDK we can't know that. Given the SDK is consumed across a wide range of unknown environments and use cases, I think we should be conservative here and not dismiss the HashDoS risk outright.

That said, I do acknowledge that most deployments are probably low-risk, dimension values typically come from controlled sources, and our cardinality capping provides an additional layer of protection. I'm supportive of giving users a performance escape hatch, I just want us to be thoughtful about how we expose it.

On the implementation approach: my concern with a crate-specific feature flag is maintenance. If we add foldhash today, we'll get requests for some other hasher tomorrow, and whatever comes after that. Over time, we could end up with a growing list of feature flags to maintain and test, each one a new dependency to vet.

A cleaner approach would be to make the internal storage generic over BuildHasher, similar to how HashMap itself works. The default stays as RandomState (SipHash), keeping the safe behavior for users who don't opt in, but users who want to bring their own hasher: foldhash, ahash, or anything else can do so without us needing to take on additional dependencies or feature flags. This does mean the generic parameter would need to thread through to MeterProvider and related types, which is a non-trivial change, but I think it's worth exploring. @bryantbiggs would you be interested in checking the feasibility of this approach?

If we do decide to go with a feature flag approach in the meantime, I'd strongly prefer the feature name include an unsafe prefix, something like unsafe-metrics-foldhash and that the docs clearly explain what the unsafe part is (no HashDoS protection), so users understand they're making a deliberate security tradeoff and not just flipping an easy performance switch.

bryantbiggs · 2026-02-24T23:45:38Z

A cleaner approach would be to make the internal storage generic over BuildHasher, similar to how HashMap itself works. The default stays as RandomState (SipHash), keeping the safe behavior for users who don't opt in, but users who want to bring their own hasher: foldhash, ahash, or anything else can do so without us needing to take on additional dependencies or feature flags. This does mean the generic parameter would need to thread through to MeterProvider and related types, which is a non-trivial change, but I think it's worth exploring. @bryantbiggs would you be interested in checking the feasibility of this approach?

Yes, I can take a look and propose something.

However, it does feel like a piece of functionality that will be commonly unknown to users. If most users don't know they can improve performance by bringing a different hash algorithm in their implementation then I would argue it's a change that is not valuable.

bryantbiggs · 2026-02-25T00:52:25Z

I dug into how every other major OTel SDK handles hashing for this exact same data structure — the per-instrument map that looks up attribute sets to aggregation buckets on every counter.add() / histogram.record() call:

Language	Data Structure	Hash Algorithm	HashDoS Resistant?	Seed
Go	`sync.Map` keyed by `Distinct{uint64}` in `limitedSyncMap`	xxHash64 (`cespare/xxhash/v2`)	No	Fixed zero
Java	`ConcurrentHashMap<Attributes, AggregatorHandle>` in `DefaultSynchronousMetricStorage`	`Arrays.hashCode()` (31×x polynomial)	No	Deterministic
C++	`unordered_map<MetricAttributes, unique_ptr<Aggregation>>` in `AttributesHashMap`	Boost `hash_combine` over `std::hash`	No	Fixed zero
Python	`dict` with `frozenset` keys in `_ViewInstrumentMatch`	Python's built-in SipHash	Yes	Per-process
.NET 6+	`ConcurrentDictionary<Tags, int>` in `AggregatorStore`	`System.HashCode` (xxHash32) + Marvin32	Yes	Per-process
Rust (current)	`RwLock<HashMap<Vec<KeyValue>, Arc<A>>>` in `ValueMap`	SipHash-1-3	Yes	Per-instance

Key takeaways:

3 of 5 other SDKs use non-DoS-resistant hashing as their only option. Python and .NET only get protection because their runtimes provide it, not from deliberate OTel decisions.
Go actively chose xxHash64 with zero seed for this path 2 months ago (PR #7497, v1.39.0), with Prometheus maintainers in the review. They even rejected collision detection because it doubled the cost of measure operations.
Go goes further than this PR — it uses only the hash as the map key (Distinct{uint64}), accepting silent data loss on collision. With foldhash, Rust's HashMap still does full Vec<KeyValue> equality comparison, so correctness is always preserved.
Zero HashDoS issues exist across the entire open-telemetry org. No spec or SIG Security guidance on hash algorithm selection. No documented real-world HashDoS attacks on observability systems.
All SDKs rely on cardinality limits (default 2000) as the primary defense.

On the BuildHasher generic approach — happy to explore as a follow-up, but I'd rather not block this on a larger API change that threads a type parameter through MeterProvider. No other OTel SDK offers configurable hashing, and the complexity may not be justified for a risk that hasn't materialized anywhere.

On naming — I'd push back on the unsafe- prefix. Go/Java/C++ ship non-resistant hashing as their only option without such labeling. metrics-foldhash with clear docs explaining the tradeoff feels more appropriate than implying a known exploitable vulnerability.

cijothomas · 2026-02-25T01:39:11Z

@bryantbiggs @utpilla "unsafe" prefix feels a bit aggressive and can incorrectly imply unsafe blocks. Okay with out unsafe prefix, but doc covering the risks.

(Btw, I don't think we can just follow what Go or C++ is doing, or go easy just because no documented exploit has occurred. As a foundation library, we need to be safe-by-default; if a particular user knows their scenario won't result in an exploitation, then can opt-in to things.)

scottgerring · 2026-03-04T12:20:48Z

Thinking out loud, could this be done via an API and an external crate / user code?
This would let users pick whatever they want, and would save even more feature flags.

bryantbiggs · 2026-03-04T13:21:28Z

Thinking out loud, could this be done via an API and an external crate / user code? This would let users pick whatever they want, and would save even more feature flags.

Yes - that is what @utpilla was describing #3388 (comment)

scottgerring · 2026-03-04T14:23:18Z

Thinking out loud, could this be done via an API and an external crate / user code? This would let users pick whatever they want, and would save even more feature flags.

whoops - accidental alignment!

cijothomas · 2026-06-02T18:45:15Z

@bryantbiggs Do you have bandwidth to check how easy it is to support bring-your-own-hash to the Metric sdk? If it is too much changes, I am okay with the feature-flag option for now. There is a risk of more users asking for different hash approaches, but lets tackle it when we actually reach there.

bryantbiggs · 2026-06-06T02:03:34Z

@utpilla @cijothomas @scottgerring — thanks for the thoughtful feedback here. I've reworked the PR to follow your suggestion: instead of a per-hasher feature flag, it now supports bring-your-own-hasher.

The metrics storage accepts any std::hash::BuildHasher via a new MeterProviderBuilder::with_hasher(...). The default stays RandomState (SipHash-1-3), so it remains safe-by-default and existing behavior is unchanged.
No new feature flags and no new SDK runtime dependencies — users bring their own hasher crate (foldhash, ahash, etc.). The previous metrics-use-foldhash feature, the foldhash dependency, and its deny.toml exception are gone.
The hasher is type-erased internally (at the existing Arc<dyn Measure> boundary), so SdkMeterProvider and MeterProviderBuilder stay non-generic — with_hasher returns the same builder type and the global dyn MeterProvider path keeps working. The public API adds exactly one method and no type parameters; code that doesn't opt in is unaffected.

I also prototyped the explicit-generic variant (SdkMeterProvider<S = RandomState> threaded through the provider, as @utpilla originally described). I ultimately went with the type-erased approach to keep a viral type parameter out of the SDK's most central public type for what is a set-once, niche performance knob — but I'm very happy to switch to the explicit generic if you'd prefer that tradeoff.

Updated design rationale and a benchmark (~29% faster counter hot path with foldhash vs. the SipHash default) are in the PR description. Thanks again for the guidance!

cijothomas · 2026-06-07T16:35:33Z

+    // On the measurement hot path the SDK looks up an aggregation bucket for
+    // each attribute set in an internal hash map. By default this uses the
+    // standard library's SipHash, which is resistant to hash-flooding (HashDoS)
+    // attacks. For high-cardinality workloads whose attribute values come from


does it matter how high the cardinality is? Hashing cost if still paid irrespective of whether attributes has high cardinality or not.

You're right — the hash is computed on every measurement regardless of cardinality, so tying the benefit to "high-cardinality" was misleading. Reworded in 070956d to frame it as a per-measurement cost: a faster hasher helps recording throughput on any hot instrument, independent of how many distinct attribute sets exist.

cijothomas · 2026-06-07T16:37:52Z

+//! Run with:
+//! ```text
+//! cargo bench --bench metrics_hasher --features metrics,experimental_metrics_custom_reader
+//! ```


nit: can you share the numbers as a comment here for quick reference. (similar to how we do for most other benchmarks)

Added in 070956d. With the single-attribute-set bench (see below), median Counter::add is ~153 ns with the default SipHash vs ~108 ns with foldhash (~30% faster) on a Threadripper 1900X.

cijothomas · 2026-06-07T16:41:19Z

+/// internal trackers map of every aggregation this builder produces. It
+/// defaults to [`RandomState`] (SipHash-1-3), preserving the DoS-resistant
+/// default; users can opt into a faster hasher via the meter provider builder.
+pub(crate) struct AggregateBuilder<T, S = RandomState> {


/// Defaults to RandomState (SipHash-1-3), which is HashDoS-resistant.
/// Use [MeterProviderBuilder::with_hasher] to install a faster hasher.

^ lets use comments like this in code docs throughout. Else this reads as PR-motivation than code documentation. (they can still be in changlog/ PR desc)

Agreed. Trimmed the AggregateBuilder doc to the terse code-doc style you suggested in 070956d.

cijothomas · 2026-06-07T16:58:13Z

-            std::mem::take(&mut *trackers)
+            // Replace with a fresh empty map that reuses the same hash builder,
+            // cloned from the current map. Using `mem::replace` (rather than
+            // `mem::take`) avoids requiring `S: Default` and preserves the


Just realized that we had a gap in this area - we used to take leaving a default HashMap in place which didn't start with the required capacity... That is an existing gap. We could fix that while we are here, or leave it for another short follow up.

Its simple enough - lets fix that in this PR itself.

Fixed in 070956d: drain_and_reset now replaces the trackers with HashMap::with_capacity_and_hasher(1 + cardinality_limit, …) instead of a zero-capacity map, so the next collection cycle refills without reallocating. Added a regression test (drain_and_reset_preallocates_tracker_capacity) that I confirmed fails on the old with_hasher version (capacity 0).

cijothomas · 2026-06-07T17:21:12Z

+                })
+            },
+            |rands| {
+                counter.add(


for demonstrating the gain of improved hashing, lets use a single attribute-set and remove the random etc, to get a pure feel of the gains by using faster hash.

Done in 070956d. Dropped the RNG and the 1600-series spread; the bench now records one fixed attribute set repeatedly, so the delta is purely the hasher cost on a steady-state lookup. That's where the ~153 ns vs ~108 ns numbers above come from.

cijothomas · 2026-06-07T17:22:50Z

    shutdown_invoked: AtomicBool,
+    /// The per-numeric-type aggregate-function builders, with the chosen hasher
+    /// already erased in. Threaded into every meter this provider creates.
+    builders: AggregateFnsBuilders,


we use the same seed for every instrument inside this provider right? Should we keep the existing behavior of generating fresh seed for each value map (i.e each instrument?)

Good catch — fixed in 070956d to preserve the per-instrument behavior for the default. The builder now stores a hasher factory (Arc<dyn Fn() -> S>) instead of one stored instance: the default uses RandomState::new, so each instrument's ValueMap gets a freshly-seeded RandomState (matching std's per-HashMap seeding and the prior behavior), while with_hasher(h) clones the user's instance so a configured/fixed-seed hasher is preserved across instruments. Added default_hasher_is_reseeded_per_instrument and custom_hasher_seed_is_preserved_across_instruments tests (the former I confirmed fails if the default falls back to a shared cloned seed).

For what it's worth on the threat model: a single per-provider seed wouldn't have been a HashDoS regression here (the seed is process-random and never exposed; std's per-map seeding is itself just a counter increment off a shared per-thread key, and it mainly guards against iteration-order leakage, which these internal maps don't expose). But restoring fresh-per-instrument for the default is essentially free and keeps behavior identical to main, so it's the cleaner choice.

cijothomas

This is awesome. Left few minor comments. Not yet marking as approved - will do another review once the comments are addressed.
Would also need another pair of eyes (anything touching metric aggregation is complex and benefits from multiple reviewers!)

Replace the proposed `metrics-use-foldhash` feature flag with support for a user-supplied `BuildHasher` on the metrics internal storage. The default remains the standard library's `RandomState` (SipHash-1-3), preserving the DoS-resistant default and existing behavior. Users with high-cardinality workloads and trusted attribute values can opt into a faster non-cryptographic hasher (e.g. foldhash, ahash) via the new `MeterProviderBuilder::with_hasher`, without adding any SDK feature flags or dependencies. The chosen hasher is monomorphized into the per-instrument `ValueMap` storage and then type-erased at the existing `Arc<dyn Measure>` boundary via a `dyn AggregateFnsBuilder` factory. As a result `SdkMeterProvider` and `MeterProviderBuilder` stay non-generic, `with_hasher` returns the same builder type, and the provider keeps working through the type-erased `dyn MeterProvider` path used by `opentelemetry::global` -- no public API generics, no change to existing code. A `metrics_hasher` benchmark compares the default hasher against foldhash (dev-dependency only) on the counter hot path.

bryantbiggs requested a review from a team as a code owner February 24, 2026 20:01

bryantbiggs force-pushed the worktree-sorted-only-valuemap branch from 159dab5 to f80ee5d Compare February 24, 2026 20:03

cijothomas reviewed Feb 24, 2026

View reviewed changes

Comment thread opentelemetry-sdk/Cargo.toml Outdated

cijothomas requested changes Feb 24, 2026

View reviewed changes

bryantbiggs force-pushed the worktree-sorted-only-valuemap branch from f80ee5d to 5e4684b Compare February 24, 2026 21:01

bryantbiggs changed the title ~~perf(sdk): use ahash for ValueMap HashMaps in metrics hot path~~ perf(sdk): add optional rapidhash for ValueMap HashMaps in metrics hot path Feb 24, 2026

bryantbiggs force-pushed the worktree-sorted-only-valuemap branch 2 times, most recently from 92730a0 to 916519b Compare February 24, 2026 21:12

bryantbiggs force-pushed the worktree-sorted-only-valuemap branch from 916519b to d58b454 Compare February 24, 2026 21:21

bryantbiggs changed the title ~~perf(sdk): add optional rapidhash for ValueMap HashMaps in metrics hot path~~ perf(sdk): add optional foldhash for ValueMap HashMaps in metrics hot path Feb 24, 2026

bryantbiggs force-pushed the worktree-sorted-only-valuemap branch 2 times, most recently from 4bd7153 to 7ce5e38 Compare March 21, 2026 01:24

bryantbiggs force-pushed the worktree-sorted-only-valuemap branch 4 times, most recently from 16a49ea to 5d181a0 Compare May 9, 2026 12:26

bryantbiggs force-pushed the worktree-sorted-only-valuemap branch from 5d181a0 to bba3af4 Compare June 5, 2026 22:20

bryantbiggs changed the title ~~perf(sdk): add optional foldhash for ValueMap HashMaps in metrics hot path~~ feat(sdk): support bring-your-own hasher for metrics via BuildHasher Jun 5, 2026

bryantbiggs force-pushed the worktree-sorted-only-valuemap branch from bba3af4 to c78a4d6 Compare June 6, 2026 01:59

cijothomas reviewed Jun 7, 2026

View reviewed changes

Comment thread opentelemetry-sdk/src/metrics/internal/mod.rs Outdated

cijothomas reviewed Jun 7, 2026

View reviewed changes

bryantbiggs force-pushed the worktree-sorted-only-valuemap branch from 5818885 to 070956d Compare June 8, 2026 16:16

bryantbiggs closed this Jun 24, 2026

Uh oh!

Conversation

bryantbiggs commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why this approach

Design / public API

Benchmark

Uh oh!

Uh oh!

cijothomas commented Feb 24, 2026

Uh oh!

cijothomas left a comment

Choose a reason for hiding this comment

Uh oh!

bryantbiggs commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

utpilla commented Feb 24, 2026

Uh oh!

bryantbiggs commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryantbiggs commented Feb 25, 2026

Uh oh!

cijothomas commented Feb 25, 2026

Uh oh!

scottgerring commented Mar 4, 2026

Uh oh!

bryantbiggs commented Mar 4, 2026

Uh oh!

scottgerring commented Mar 4, 2026

Uh oh!

cijothomas commented Jun 2, 2026

Uh oh!

bryantbiggs commented Jun 6, 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!

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!

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!

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!

cijothomas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bryantbiggs commented Feb 24, 2026 •

edited

Loading

bryantbiggs commented Feb 24, 2026 •

edited

Loading

codecov Bot commented Feb 24, 2026 •

edited

Loading

bryantbiggs commented Feb 24, 2026 •

edited

Loading

bryantbiggs commented Jun 6, 2026 •

edited

Loading