[OTLP] Use packed format for metric histograms #6567
Conversation
github-actions
bot
added
the
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6567 +/- ##
==========================================
+ Coverage 86.64% 86.77% +0.13%
==========================================
Files 258 258
Lines 11910 11946 +36
==========================================
+ Hits 10319 10366 +47
+ Misses 1591 1580 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
martincostello
changed the title
Consume changes from open-telemetry/opentelemetry-dotnet#6567 for testing.
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/IntegrationTest/docker-compose.yml
Outdated
Show resolved
Hide resolved
Consume changes from open-telemetry/opentelemetry-dotnet#6567 for testing.
Add entry for open-telemetry#6567.
martincostello
force-pushed
the
gh-6538
branch
from
298a1a5 to
9c0aba8
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR updates the OTLP metric histogram serialization to use packed format instead of unpacked format, fixing compatibility issues where some OTLP receivers were rejecting payloads with HTTP 400 responses due to gRPC protocol errors.
Key Changes:
- Refactored histogram bucket serialization from individual field writes to packed format
- Added helper method
WriteDoublefor packed double serialization - Replaced inline histogram bucket writing with dedicated
WriteHistogramBucketsmethod
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ProtobufSerializer.cs | Added WriteDouble helper method for packed double serialization |
| ProtobufOtlpMetricSerializer.cs | Refactored histogram bucket serialization to use packed format with dedicated helper methods |
| CHANGELOG.md | Added entry documenting the fix for OTLP receiver rejection issues |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Use packed format for metric histograms. Resolves open-telemetry#6538.
martincostello
force-pushed
the
gh-6538
branch
from
873eb3b to
1ba2f0c
Compare
|
Given that the OTLP integration tests pass without this change, I'll try and create a unit test that validates the layout of the serialized protobuf message is in the expected packed format. |
Add snapshot test for the protobuf serialized metrics.
github-actions
bot
added
infra
Resolve IDE0005 warnings caused by Verify adding an implicit using for Xunit.
| public static class ProtobufOtlpMetricSerializerTests | ||
| { | ||
| [Fact] | ||
| public static async Task WriteMetricsData_Serializes_Metrics_Correctly() |
There was a problem hiding this comment.
I've verified manually that if this test is added to main it fails.
|
I also tried a test where the assertion was the below, but the Google.Protobuf library appears to be lenient enough to support packed and unpacked fields, so it also passes in var deserialized = Proto.Metrics.V1.MetricsData.Parser.ParseFrom(buffer, 0, actual);
Assert.NotNull(deserialized);
Assert.NotNull(deserialized.ResourceMetrics);
Assert.NotEmpty(deserialized.ResourceMetrics);
foreach (var resourceMetrics in deserialized.ResourceMetrics)
{
foreach (var scope in resourceMetrics.ScopeMetrics)
{
foreach (var metric in scope.Metrics)
{
if (metric.Histogram is { } histogram)
{
foreach (var point in histogram.DataPoints)
{
Assert.NotNull(point.BucketCounts);
Assert.NotEmpty(point.BucketCounts);
Assert.NotNull(point.ExplicitBounds);
Assert.NotEmpty(point.ExplicitBounds);
}
}
}
}
} |
Move the code to generate the `Batch<Metrics>` to another method.
| this.StartTimeExclusive = startTimeExclusive; | ||
| this.EndTimeInclusive = endTimeInclusive; |
There was a problem hiding this comment.
Any option to make this calls by the reflection?
There was a problem hiding this comment.
I can do that if desired, but IMHO that's a bit yuck if the tests already have [InternalsVisibleTo]. Or alternatively I could just make the setters internal instead of prviate.
Fixes #6538
Changes
Updates the code that writes the protobuf fields for metrics histograms to use packed format instead of unpacked.
TODO
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)Benchmarks
Manually edited this code:
opentelemetry-dotnet/test/Benchmarks/Exporter/OtlpGrpcExporterBenchmarks.cs
Line 26 in 6a70665
to:
Otherwise, it takes 75 minutes just to get to the warm-up phase for the
100iteration.This PR (0bdac99)
gRPC
Command:
HTTP
Command:
main (6a70665)
gRPC
Command:
HTTP
Command:
End-to-end testing
I ran two Grafana k6 performance tests against an application of my own with the following two commits:
Both runs have similar performance profiles, and metrics histograms render similarly.
Comparison
v1.13.0
This PR (0bdac99)