No memory leakage in attributes filter

[regeda]

The attributes filter collects seen attributes in order to avoid filtration on the same attribute set. However, the attribute.Set is not comparable type and new allocations of sets with same attributes will be considered as new sets.

Metrics with a high cardinality of attributes consume a lot of memory even if we set a filter to reduce that cardinality.

[MrAlias]

However, the attribute.Set is not comparable type

Hmm, that's new. I guess if you have more than 10 attributes or those attributes have the new slice values they will compare the pointer values not the underlying values. This is a bug for other parts of the SDK as well.

[MrAlias]

This seems reasonable to me.

If I recall correctly this map was added to act as a cache and reduce computation. If removing it helps reduce ever-expanding memory overhead at the cost of having to calculate the filter each time that seems reasonable to me.

@MadVikingGod thoughts?

[codecov]

Codecov Report

Merging #3695 (ca5e08f) into main (f5a1497) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #3695     +/-   ##
=======================================
- Coverage   79.7%   79.6%   -0.1%     
=======================================
  Files        171     171             
  Lines      12673   12657     -16     
=======================================
- Hits       10103   10085     -18     
- Misses      2357    2359      +2     
  Partials     213     213             

Impacted Files	Coverage Δ
sdk/metric/internal/filter.go	100.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go	80.2% <0.0%> (-1.8%)	⬇️
exporters/jaeger/jaeger.go	91.1% <0.0%> (+0.8%)	⬆️

[jmacd]

Aside:

However, the attribute.Set is not comparable type and new allocations of sets with same attributes will be considered as new sets.

I do not believe this is true. The attribute.Set mechanism constructs a comparable array of KeyValue. For slice-valued Values, this process is carried out recursively -- slices are represented as arrays so they can be comparable.

The attribute.Set logic is tested correct above 10 attributes, it just falls back to use of reflect for >10 attributes in a set. The cost of attribute set construction is the cost of constructing a specific-size array of KeyValue.

If there's a bug in this logic, we should fix it -- or was the underlying issue really about accumulation of distinct sets? I take it from the fix in this PR that the underlying issue was true cardinality, not a failure of the attribute.Set logic. I'll be glad to help if there's a problem!

[MrAlias]

Aside:

However, the attribute.Set is not comparable type and new allocations of sets with same attributes will be considered as new sets.

I do not believe this is true. The attribute.Set mechanism constructs a comparable array of KeyValue. For slice-valued Values, this process is carried out recursively -- slices are represented as arrays so they can be comparable.

The attribute.Set logic is tested correct above 10 attributes, it just falls back to use of reflect for >10 attributes in a set. The cost of attribute set construction is the cost of constructing a specific-size array of KeyValue.

If there's a bug in this logic, we should fix it -- or was the underlying issue really about accumulation of distinct sets? I take it from the fix in this PR that the underlying issue was true cardinality, not a failure of the attribute.Set logic. I'll be glad to help if there's a problem!

Indeed #3702

We double checked this and the latest version of attribute.Set is comparable.