Skip to content
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

OTLP exporter: Fix TypeInitializationException thrown by exporter #1873

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Mar 6, 2021

Fixes #1854.

Uses new UnsafeByteOperations.UnsafeWrap method in Google.Protobuf 3.15+ to avoid additional byte[] allocation.

Perf before:

Method NumberOfBatches NumberOfSpans Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
OtlpExporter_Batching 1 10000 83.97 ms 1.561 ms 1.533 ms 4166.6667 1500.0000 500.0000 21.29 MB
OtlpExporter_Batching 10 10000 878.07 ms 16.511 ms 27.129 ms 47000.0000 19000.0000 7000.0000 212.88 MB
OtlpExporter_Batching 100 10000 9,006.40 ms 49.816 ms 44.160 ms 509000.0000 201000.0000 89000.0000 2128.82 MB

Perf after:

Method NumberOfBatches NumberOfSpans Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
OtlpExporter_Batching 1 10000 83.43 ms 1.487 ms 1.318 ms 4500.0000 1833.3333 666.6667 21.67 MB
OtlpExporter_Batching 10 10000 872.01 ms 13.699 ms 11.440 ms 45000.0000 18000.0000 7000.0000 216.7 MB
OtlpExporter_Batching 100 10000 9,164.78 ms 122.865 ms 108.917 ms 547000.0000 222000.0000 99000.0000 2166.95 MB

Seems there is a bit of an increase in memory allocated... I will investigate why.

@alanwest alanwest requested a review from a team as a code owner March 6, 2021 21:53
@alanwest alanwest changed the title Fix TypeInitializationException thrown by exporter OTLP exporter: Fix TypeInitializationException thrown by exporter Mar 6, 2021
@codecov
Copy link

codecov bot commented Mar 6, 2021

Codecov Report

Merging #1873 (113c390) into main (9ebc1ce) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1873      +/-   ##
==========================================
- Coverage   83.56%   83.56%   -0.01%     
==========================================
  Files         192      192              
  Lines        6163     6150      -13     
==========================================
- Hits         5150     5139      -11     
+ Misses       1013     1011       -2     
Impacted Files Coverage Δ
...metryProtocol/Implementation/ActivityExtensions.cs 84.93% <100.00%> (-0.85%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+5.88%) ⬆️

@alanwest
Copy link
Member Author

@CodeBlanch @cijothomas @reyang

I fairly certain the increase in memory allocated is not due to the code changes here. The new UnsafeByteOperations.UnsafeWrap method avoids an additional allocation of a byte array. The original code was achieving this by reflection which breaks consumers of the Google.Protobuf package version 3.15+.

I suspect that the increase in allocation is related to upgrading our dependency to 3.15.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@CodeBlanch
Copy link
Member

CodeBlanch commented Mar 31, 2021

LGTM I don't think the change is the reason for the perf hit:

https://github.com/protocolbuffers/protobuf/blob/e9360dfa53f151e64c996823785e0eb6c8118620/csharp/src/Google.Protobuf/UnsafeByteOperations.cs#L78

https://github.com/protocolbuffers/protobuf/blob/e9360dfa53f151e64c996823785e0eb6c8118620/csharp/src/Google.Protobuf/ByteString.cs#L65

That all seems perfectly reasonable to me. Assuming it is flowing through that ReadOnlyMemory<byte> due to implicit cast from byte[].

@cijothomas cijothomas merged commit ec9f2d4 into open-telemetry:main Mar 31, 2021
@alanwest alanwest deleted the alanwest/otlp-fix-typeinitializationexception branch April 1, 2021 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google.Protobuf 3.15.0 breaks the Otlp Exporter
4 participants