[OTLP] Cache metric metadata bytes for protobuf serializer#7307
[OTLP] Cache metric metadata bytes for protobuf serializer#7307unsafePtr wants to merge 5 commits into
Conversation
|
Would be good to see some before/after numbers before merge. |
cijothomas
left a comment
There was a problem hiding this comment.
Need to see if this "leaks" Metric.
Unlike Resource (effectively a per-provider singleton), Metric instances come and go. A static ConcurrentDictionary<Metric, byte[]> pins every Metric it ever sees for the process lifetime, leaking memory for apps.
See if this makes sense and see if we can do alternates like ConditionalWeakTable<Metric, byte[]>, or an internal field on Metric itself.?
|
Benchmark results for the metric metadata cache. Before (no cache)
After (with cache)
Delta
Steady-state savings of ~12 ns per metric in the metadata write path. Zero allocations either way — the original serializer was already alloc-free; this PR cuts CPU work, not memory. |
|
@martincostello , @cijothomas generally I was expecting way smaller perf benefit here, comparing to Resource. If ~20ns win doesn't justify code changes, please feel free to close the PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7307 +/- ##
==========================================
- Coverage 89.94% 89.93% -0.02%
==========================================
Files 276 276
Lines 14007 14032 +25
==========================================
+ Hits 12599 12619 +20
- Misses 1408 1413 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
4fdb347 to
84389ad
Compare
Match dotnet/runtime convention (StackallocThreshold = 256) and use separate constants for the stack and pool initial sizes. Addresses open-telemetry#7307 (comment)
|
This looks good to me so far. I defer to @cijothomas being happy regarding the lifetimes before sticking a ✅ on this PR though. |
Pre-serialize each Metric's name/description/unit once into a per-metric byte cache keyed on the Metric instance. Hot-path emits the cached blob with a single buffer copy instead of re-encoding strings on every export. Cache is a ConditionalWeakTable<Metric, byte[]> so cache entries are released with the metric. Stackalloc with an ArrayPool fallback covers oversized metadata. Adds polyfills (scoped via preprocessor) so the change builds on legacy targets: - ConditionalWeakTable.GetOrAdd (added in .NET 10) for non-net10 TFMs - Encoding.GetBytes(ReadOnlySpan<char>, Span<byte>) for netstandard2.0 / net462 - DynamicallyAccessedMembersAttribute / DynamicallyAccessedMemberTypes for any non-.NET 5+ TFM (netstandard2.1 ships them internal-only)
Match dotnet/runtime convention (StackallocThreshold = 256) and use separate constants for the stack and pool initial sizes. Addresses open-telemetry#7307 (comment)
Mirrors the WriteResourceDoesNotKeepResourceAlive coverage added in open-telemetry#7326 for the resource cache. Guards against accidental regression from the weak-keyed ConditionalWeakTable to a strong-keyed cache.
Drop the #if NET10_0_OR_GREATER preprocessor in the Resource cache call site and rely on the shared ConditionalWeakTableExtensions.GetOrAdd polyfill that backs the Metric cache. Single, consistent call shape across the two serializers.
4275fb2 to
05508b8
Compare
|
Updated changes coming from #7326. Unified pattern under common GetOrAdd polyfill for ConditionalWeakTable |
Follow-up to #7303 which applied the same pattern to the resource bytes.
Changes
Caches the protobuf-encoded bytes of
Metric.Name/Description/Unitkeyed byMetricinstance, replayed on every export viaBuffer.BlockCopy. These three fields are set at instrument creation and immutable for the lifetime of aMetric, so the bytes never change after first computation.Implementation mirrors
ProtobufOtlpResourceSerializer:ConcurrentDictionary<Metric, byte[]>static cache.ArrayPool<byte>-backed scratch buffer with grow-on-overflow retry for the one-shot slow path.GetOrAdd+Buffer.BlockCopy.Merge requirement checklist
OtlpMetricsExporterTestsandProtobufOtlpMetricSerializerTestscover the new path.CHANGELOG.mdfiles updated for non-trivial changes