-
Notifications
You must be signed in to change notification settings - Fork 891
[OTLP] Cache resource bytes for protobuf serializer #7303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
martincostello
merged 10 commits into
open-telemetry:main
from
unsafePtr:perf/pre-serialize-otlp-resource
May 18, 2026
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
fc4c2e7
feat: pre-cache resource bytes for protobuf oltp serializer
unsafePtr 7329d50
docs: add changelog entry for resource bytes cache
unsafePtr 20db64f
refactor: cache ArrayPool<byte>.Shared into a local
unsafePtr b34f252
perf: write empty-resource bytes inline via ReadOnlySpan literal
unsafePtr 48b546c
perf: lower initial scratch size to 2 KB
unsafePtr e80ce52
test: parameterize resource benchmark by attribute count
unsafePtr dd5e2d2
docs: move changelog entry to end of unreleased section
unsafePtr f6e12d6
fix: avoid range syntax in test for net462 compatibility
unsafePtr 4f1d8a5
refactor: address review nits — rename locals and tighten test buffer
unsafePtr 7f4816d
Merge remote-tracking branch 'origin/main' into perf/pre-serialize-ot…
unsafePtr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
82 changes: 82 additions & 0 deletions
82
test/Benchmarks/Exporter/ProtobufOtlpResourceSerializerBenchmarks.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| extern alias OpenTelemetryProtocol; | ||
|
|
||
| using BenchmarkDotNet.Attributes; | ||
| using OpenTelemetry.Resources; | ||
| using OpenTelemetryProtocol::OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.Serializer; | ||
|
|
||
| namespace Benchmarks.Exporter; | ||
|
|
||
| public class ProtobufOtlpResourceSerializerBenchmarks | ||
| { | ||
| private static readonly KeyValuePair<string, object>[] AllAttributes = | ||
| [ | ||
| new("service.name", "checkout-api"), | ||
| new("service.namespace", "shop"), | ||
| new("service.version", "2.4.1"), | ||
| new("service.instance.id", "9a8d1f3e-4b2c-4e15-9a4f-1b2c3d4e5f6a"), | ||
| new("telemetry.sdk.name", "opentelemetry"), | ||
| new("telemetry.sdk.language", "dotnet"), | ||
| new("telemetry.sdk.version", "1.13.0"), | ||
| new("deployment.environment", "production"), | ||
| new("host.name", "ip-10-0-12-47.eu-west-1.compute.internal"), | ||
| new("host.id", "i-0abcdef1234567890"), | ||
| new("host.type", "c6a.2xlarge"), | ||
| new("host.arch", "amd64"), | ||
| new("os.type", "linux"), | ||
| new("os.description", "Ubuntu 22.04.4 LTS"), | ||
| new("process.pid", 18742L), | ||
| new("process.executable.name", "Checkout.Api"), | ||
| new("process.runtime.name", ".NET"), | ||
| new("process.runtime.version", "10.0.8"), | ||
| new("container.id", "8f3a7b4c9e1d2f5a6b8c0e2f4a6b8d0c2e4f6a8b0c2d4e6f8a0b2c4d6e8f0a2b"), | ||
| new("container.image.name", "registry.example.com/shop/checkout-api"), | ||
| new("container.image.tag", "2.4.1-abc1234"), | ||
| new("k8s.namespace.name", "shop-prod"), | ||
| new("k8s.pod.name", "checkout-api-7d8c9f4b6c-x9k2m"), | ||
| new("k8s.pod.uid", "d4e5f6a7-b8c9-4d0e-9f1a-2b3c4d5e6f7a"), | ||
| new("k8s.node.name", "ip-10-0-12-47.eu-west-1.compute.internal"), | ||
| new("k8s.deployment.name", "checkout-api"), | ||
| new("k8s.cluster.name", "shop-prod-eu-west-1"), | ||
| new("cloud.provider", "aws"), | ||
| new("cloud.region", "eu-west-1"), | ||
| new("cloud.availability_zone", "eu-west-1a"), | ||
| new("cloud.account.id", "123456789012"), | ||
| new("deployment.id", "deploy-2024-05-17-abc"), | ||
| ]; | ||
|
|
||
| private readonly byte[] buffer = new byte[64 * 1024]; | ||
| private Resource resource = Resource.Empty; | ||
|
|
||
| [Params(0, 4, 8, 16, 32)] | ||
| public int AttributeCount { get; set; } | ||
|
|
||
| [GlobalSetup] | ||
| public void Setup() | ||
| { | ||
| if (this.AttributeCount == 0) | ||
| { | ||
| this.resource = Resource.Empty; | ||
| return; | ||
| } | ||
|
|
||
| var attributes = new Dictionary<string, object>(this.AttributeCount); | ||
| var count = Math.Min(this.AttributeCount, AllAttributes.Length); | ||
| for (var i = 0; i < count; i++) | ||
| { | ||
| attributes[AllAttributes[i].Key] = AllAttributes[i].Value; | ||
| } | ||
|
|
||
| for (var i = count; i < this.AttributeCount; i++) | ||
| { | ||
| attributes[$"custom.attribute.{i}"] = Guid.NewGuid().ToString(); | ||
| } | ||
|
|
||
| this.resource = new Resource(attributes); | ||
| } | ||
|
|
||
| [Benchmark] | ||
| public int WriteResource() => ProtobufOtlpResourceSerializer.WriteResource(this.buffer, 0, this.resource); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsure if static is the right choice here. if we store it in MeterProvider or exporter itself, then we can avoid the Dictionary completely as there is single Resource associated with an exporter always.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you mean move the cache up one level to the caller?
That sounds similar to something I've done in #7279 to cache Prometheus metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cijothomas , Resource is immutable, isn't it? static dictionary is the right choice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Immutability of Resource isn't really the concern — the issue is lifetime. A static ConcurrentDictionary<Resource, byte[]> strongly references every Resource instance the process ever sees, including ones from disposed MeterProviders. For Resource it's usually bounded in practice so the leak is small, but caching on the exporter/provider (where there's a single Resource per exporter) would avoid the dictionary entirely and bound the lifetime correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it, but the changes surface was quite big, plus same bytes would be effectively duplicated across 3 exporters. While static dictionary is a simple change and I thought that Resource reference is typically living for a lifetime of the application. Several dangling references for a long-running processes, is usually acceptable. Still it's quite easy to swap ConcurrentDicitonary with ConditionalWeakTable here with loosing a bit on perf if dangling references are problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. This one has low risk, but it'd be still better to switch to ConditionalWeakTable here in a follow up PR. I am more worried about the Metric/scope change PR (#7307), as that has more real leak risks.