Improve performance of attribute.NewSet

[dashpole]

I was going to write a blog about the metric SDK optimization work, and was working on updating https://github.com/promlabs/prometheus-otel-benchmarks to demonstrate the improvements. I tried adding a benchmark that follows our best practices for performance in promlabs/prometheus-otel-benchmarks@2c01c31, but I still see an allocation in attribute.NewSet.

I confirmed that is the case in the BenchmarkNewSet benchmark here:

BenchmarkNewSet                  	  233108	       503.3 ns/op	     448 B/op	       1 allocs/op

Unfortunately, this mostly makes the optimizations in the metrics SDK a bit irrelevant. A counter increment is now ~60 ns and has no allocations, but creating the attribute set to pass to it is 500ns with an allocation. The SDK can be used in a performant way, but only if the user precomputes all commonly used attribute sets.

Ideas

I was playing around with some ideas, but I haven't been able to get any to work yet.

One idea is to introduce a constructor for attribute.Set that accepts another attribute.Set as storage:

func NewReusedSet(existing *Set, kvs KeyValue...) *Set {}

The idea would be that the user can create (yet another) sync.Pool, and manage the re-use of sets with the same number of underlying attributes. This has the advantage of providing users a way to re-use sets without the risk of a different library causing issues.

Another idea is to add Set.Free():

//  Free releases the underlying storage of the Set to allow it to be re-used by subsequent NewSet calls.
func (s *Set) Free() {}

which puts the set in a pool of sets with the same number of attributes. I prototyped this here: main...dashpole:opentelemetry-go:noalloc_set, but wasn't able to get it to work properly, while still keeping Set comparable.

I'm looking for other ideas as well. I think this is a big enough performance penalty for us to even consider a v2 of the attributes package, if necessary. Currently, writing performant metrics code is quite onerous: https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md#attribute-and-option-allocation-management, and this may be an opportunity to address that as well.

"Bound" instruments could also help here, but will miss a number of common use-cases for metrics libraries, so I don't consider that a complete solution.

[bboreham]

only if the user precomputes all commonly used attribute sets

Could you add some words about why this is not a reasonable solution?

• I am reminded of otelgrpc: optimize stats handler InPayload and OutPayload opentelemetry-go-contrib#8035, which might form a good example.

[dashpole]

Could you add some words about why this is not a reasonable solution?

It might be the best solution we can come up with. I mostly don't like that writing performant metric instrumentation is complex, and very verbose if you need to precompute a large number of permutations. Documentation can help with this, but it feels like the library is very "slow by default", if you use the APIs the way you would intuitively use them.

[dashpole]

One of the key reasons why the Prometheus API doesn't have an allocation is because if the attribute set is already stored in the SDK, it doesn't need to allocate memory for the new one. It just needs to verify that the provided attributes match what has already been stored and can then return. In our case, this would means that only the Distinct (hash) of the attribute set is actually used for all instances of an attribute set after the first. In an ideal world, we would accept []KeyValue (somehow), compute the hash, and then only copy/store data if the attribute set is new.

From our benchmarks, just computing the hash is usually ~200 ns, and 0 allocs. Creating the full "new set" is 500 ns and 1 alloc.

Design ideas:

• Could we add a constructor for Distinct directly using KeyValue...? The metrics API could accept ...KeyValue, first compute the Distinct, and only compute the full NewSet if needed. We could add AddWithAttributes functions to the API? Seems potentially worth it.
• Could we lazily initialize the NewSet, and keep a reference to the original kvs until we actually need them? On the first usage of data, we would initialize it. Seems potentially unsafe if users modify the keyValues prior to using them with the SDK, but seems like an odd usage pattern.

I'm sure others have ideas too.

[dashpole]

I'm fairly pleased with the benchmark results of the AddWithAttributes prototype: #7770. I'll plan to discuss it at the SIG meeting next week.

In all cases, it has 0 allocations instead of 2 allocations for the current "optimized" code.

For 1 attribute, it is 3x faster overall, at 63 ns vs 170 ns.

For 3 attributes, it is 2x faster overall, at 141 ns vs 292 ns.

For 10 attributes, it is 2x faster overall, at 365 ns vs 816 ns.

[dashpole]

Thinking about this even more, I think we should ideally combine this change with a switch to "bound attributes" for synchronous instruments. This design largely follows the design of the Prometheus SDK.

Usage

For "dynamic" attributes:

myCounter.WithAttributes(attrs...).Add(ctx, 1)

For "precomputed" attributes:

successCounter := myCounter.WithAttributes(componentTypeAttr, noErrorTypeAttr)
// ... later, in the hot path ...
successCounter.Add(ctx, 1)

For a mix of precomputed and dynamic attributes:

successCounter := myCounter.WithAttributes(componentTypeAttr, noErrorTypeAttr)
// ... later, in the hot path, when a failure occurs, overwrite error.type ...
successCounter.WithAttributes(yesErrorTypeAttr).Add(ctx, 1)

API Changes

Introduce a WithAttributes function on all instruments:

// Float64Counter is an instrument that records increasing float64 values.
//
// Warning: Methods may be added to this interface in minor releases. See
// package documentation on API implementation for information on how to set
// default behavior for unimplemented methods.
type Float64Counter interface {
	// Users of the interface can ignore this. This embedded type is only used
	// by implementations of this interface. See the "API Implementations"
	// section of the package documentation for more information.
	embedded.Float64Counter

	// Add records a change to the counter.
	//
	// Use the WithAttributeSet (or, if performance is not a concern,
	// the WithAttributes) option to include measurement attributes.
	Add(ctx context.Context, incr float64, options ...AddOption)

+   // WithAttributes binds attributes to the returned Float64Counter.
+   // Subsequent calls to Add will include the provided attributes.
+   // Calling WithAttributes on the returned counter appends additional
+   // attributes, or overrides existing attributes if they are already present.
+   WithAttributes(kvs ...KeyValue) Float64Counter
}

Deprecate the following from our public APIs:

• metric.WithAttributeSet
• attribute.Distinct (can be made internal to the metrics SDK).
• attribute.Set (can be made internal to the metrics SDK).

SDK Implementation + Optimization

WithAttributes would append the kvs to the existing attributes for the instrument (if previously bound), hash the kvs provided, and perform the "map lookup" in the aggregation. It would only compute a "NewSet", if the element is not found in the map. Add (assuming no attributes are provided) would no longer need to perform a map lookup. It can just increment an atomic counter, which is very fast.

Rationale

This has many advantages:

• Improve the performance of the "precomputed" case, since we avoid the map lookup.
• Improve the performance of the "dynamic" attributes case, since we don't need to create NewSet for pre-existing attribute sets.
• Improve the readability of code in all cases. Storing references to bound attribute sets is simple. Being able to append a dynamic attribute to an existing bound instrument means the "dynamic" part is more obvious.
• The addition of a new function on instruments is a "clean" addition. We don't need a second "flavor" of add, like AddWithAttributes.
• Simplify the attributes package by deprecating Distinct and Set. All signals now depend only on KeyValue.

Disadvantages:

The SDK implementation would remain fairly complex, as we still need to be able to support metric.WithAttributeSet. We could possibly change WithAttributeSet to just be a wrapper around the new WithAttributes, but that would incur an additional performance penalty for users when we make that change (if they don't migrate).

Proposed implementation plan

1. Add a "MAY" clause to the OpenTelemetry specification to allow bound instruments.
2. Once this clause stabilises, add the new WithAttributes function to all instruments in the API + SDK, without impacting additional performance.
3. Migrate all instrumentation to the new APIs, and wait for feedback
4. Deprecate the old options and types.
5. Make metric.WithAttributeSet a wrapper around the new WithAttributes, and simplify the SDK.
6. (maybe someday) Release a v2 without the deprecated types.

[jmacd]

This looks great!

[MrAlias]

Thinking about this even more, I think we should ideally combine this change with a switch to "bound attributes" for synchronous instruments. This design largely follows the design of the Prometheus SDK.

From prior experience instrumenting our SDK, this didn't win us much. I built https://github.com/MrAlias/bind but found that I didn't use it at all. If you are going to bind attributes the only difference it makes is that some other code tracks the creation and association of an attribute.Set. There is no difference in performance than just having a user create the attributes set they want and pass it when the instrument is called.

This does not help with the problematic performance issue of dynamic attributes which is where the attribute.Set has actual performance issues.

[dashpole]

@MrAlias I thought that too, until my realization in #7743 (comment). We only need to compute the hash in the common case, and don't need to do all the other stuff in NewSet unless this is the first time the SDK has seen the set. See the benchmark results in #7743 (comment) for evidence. The key code change in the SDK is here: 2045b6f#diff-717b185025e1672b29f6a04583801486bab6f517206e6584593e903ab8da3d77R32-R48.

[MrAlias]

yeah ... I think the benchmarks are misleading. They are comparing a case where the same set is reused over and over with one where it is created each run. They are obvious results based on that and I don't think they are reflective of a truly dynamic attribute situation.

[dashpole]

Alright, I now have another alternative to consider, which is an improvement, but doesn't (yet) fully solve the problem: #7777. It basically makes the existing metric.WithAttributes() much more performant for the dynamic case. The only problem is that it still has an allocation, which I would love to get rid of, and it is still quite involved to use in a performant way.

[cijothomas]

yeah ... I think the benchmarks are misleading. They are comparing a case where the same set is reused over and over with one where it is created each run. They are obvious results based on that and I don't think they are reflective of a truly dynamic attribute situation.

https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/benches/metrics_counter.rs#L90-L98
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/test/Benchmarks/Metrics/MetricsBenchmarks.cs#L129

For OTel Rust/.NET, we try to benchmarks with "random" attributes. (In case you want to copy some ideas)