[Do not Merge] [Otlp Exporter] Remove Google.Protobuf and Grpc.Net.Client dependency #5731

vishweshbankwar · 2024-07-01T16:31:23Z

Changes

Removes Google.Protobuf dependency for serialization for Trace exporter. Implements a custom Serializer as per the OTLP spec.
For Grpc, uses HttpClient for sending HTTP/2 requests. Dependency on Grpc.Core.Api is kept as is for error handling (creating RpcExceptions) (Can be removed as well by forking certain types).
Custom implementation is put behind a feature flag OTEL_DOTNET_EXPERIMENTAL_USE_CUSTOM_PROTOBUF_SERIALIZER for now.

Note: This is a proof of concept PR to collect the initial feedback and is not intended to be merged. Smaller follow-up PRs will be done once the approach is finalized.

Merge requirement checklist

CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

…ependency

vishweshbankwar · 2024-07-01T16:37:24Z

...emetry.Exporter.OpenTelemetryProtocol/Implementation/Custom/Serializer/ActivitySerializer.cs

+            if (attributeCount < maxAttributeCount)
+            {
+                /*
+                    // Alternate approach


@CodeBlanch - Could you take a look at this to see if it matches your expectation? I have not done the complete implementation yet, but can change if the direction looks good to you.

codecov · 2024-07-01T16:40:38Z

Codecov Report

Attention: Patch coverage is 76.60377% with 248 lines in your changes missing coverage. Please review.

Project coverage is 85.31%. Comparing base (6250307) to head (5e78598).
Report is 298 commits behind head on main.

Files	Patch %	Lines
...entation/Custom/ExportClient/GrpcProtocolHelper.cs	57.01%	49 Missing ⚠️
...rotocol/Implementation/Custom/Serializer/Writer.cs	54.71%	48 Missing ⚠️
.../Implementation/Custom/Serializer/OtlpTagWriter.cs	0.00%	41 Missing ⚠️
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs	25.00%	36 Missing ⚠️
...ation/Custom/Serializer/WireTypesSizeCalculator.cs	29.03%	22 Missing ⚠️
...ol/Implementation/Custom/Transmission/OtlpRetry.cs	77.27%	15 Missing ⚠️
...ementation/Custom/Serializer/ActivitySerializer.cs	94.53%	10 Missing ⚠️
...tlpExporterPersistentStorageTransmissionHandler.cs	86.15%	9 Missing ⚠️
...tation/Custom/ExportClient/OtlpGrpcExportClient.cs	86.53%	7 Missing ⚠️
...om/Transmission/OtlpExporterTransmissionHandler.cs	88.23%	4 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5731      +/-   ##
==========================================
+ Coverage   83.38%   85.31%   +1.93%     
==========================================
  Files         297      273      -24     
  Lines       12531    12103     -428     
==========================================
- Hits        10449    10326     -123     
+ Misses       2082     1777     -305

Flag	Coverage Δ
unittests	`?`
unittests-Project-Experimental	`85.31% <76.60%> (?)`
unittests-Project-Stable	`85.42% <76.60%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...tation/Custom/Serializer/ActivitySizeCalculator.cs	`100.00% <100.00%> (ø)`
...ntation/Custom/Serializer/CommonTypesSerializer.cs	`100.00% <100.00%> (ø)`
.../Implementation/Custom/Transmission/RetryHelper.cs	`100.00% <100.00%> (ø)`
...etryProtocol/Implementation/ExperimentalOptions.cs	`90.90% <100.00%> (-9.10%)`	⬇️
...metryProtocol/OtlpTraceExporterHelperExtensions.cs	`97.33% <100.00%> (+0.66%)`	⬆️
...rter.OpenTelemetryProtocol/OtlpTraceExporterNew.cs	`100.00% <100.00%> (ø)`
...tion/Custom/ExportClient/TrailingHeadersHelpers.cs	`50.00% <50.00%> (ø)`
...ion/Custom/Serializer/CommonTypesSizeCalculator.cs	`98.00% <98.00%> (ø)`
...ansmission/OtlpExporterRetryTransmissionHandler.cs	`80.00% <80.00%> (ø)`
...tation/Custom/ExportClient/OtlpHttpExportClient.cs	`91.89% <91.89%> (ø)`
... and 10 more

... and 226 files with indirect coverage changes

Kielek · 2024-07-03T11:14:56Z

...etry.Exporter.OpenTelemetryProtocol/Implementation/Custom/ExportClient/GrpcProtocolHelper.cs

+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+// Includes work from:


I think that it will require us to extend LICENCSE.md/create rd-party notice and include it into nuget package. It is required by Apache license,

We actually already have one: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/THIRD-PARTY-NOTICES.TXT

Both LICENSE.txt and THIRD-PARTY-NOTICES.TXT get included in the packages today. Ex: https://nuget.info/packages/OpenTelemetry.Exporter.OpenTelemetryProtocol/1.9.0

rajkumar-rangaraj · 2024-07-08T18:30:02Z

Directions look good to me. When you plan to create smaller PRs, consider adding detailed comments at the top of every method to help ease the review and later debugging process, as most of the methods perform calculations.

rajkumar-rangaraj · 2024-07-08T18:34:32Z

...emetry.Exporter.OpenTelemetryProtocol/Implementation/Custom/Serializer/ActivitySerializer.cs

+            }
+            else
+            {
+                if (!ActivityListPool.TryTake(out var newList))


Do we need such complex storage? I understand that we need to track a single ActivitySource.Name for the instrumentation scope. Can we rely on Activity.Source and a simple thing like isSeen during the serialization process?

CodeBlanch · 2024-07-08T19:13:18Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/Custom/Serializer/Writer.cs

+        ref byte[] buffer,
+        int cursor,
+        int fieldNumber,
+        ReadOnlySpan<char> value,


@vishweshbankwar I pushed an update so Writer understands how to write a ReadOnlySpan<char> to the buffer. Previously we called ToString in TagWriter which would have caused a lot of extra allocations. Also updated this so it calls into the code to grow the buffer if needed before writing instead of having two branches with one performing that check per-byte/char written. Should be much faster this way and I think simpler.

github-actions · 2024-07-16T03:18:12Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

github-actions · 2024-07-23T03:20:12Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

JamesNK · 2024-08-06T13:17:43Z

...etry.Exporter.OpenTelemetryProtocol/Implementation/Custom/ExportClient/GrpcProtocolHelper.cs

+                if (isBrowser)
+                {
+                    detail += " If the gRPC call is cross domain then CORS must be correctly configured. Access-Control-Expose-Headers needs to include 'grpc-status' and 'grpc-message'.";
+                }
+
+                if (isWinHttp)
+                {
+                    detail += " Using gRPC with WinHttp has Windows and package version requirements. See https://aka.ms/aspnet/grpc/netstandard for details.";
+                }


You don't need to worry about grpc-web or CORS.
Do you plan on supporting WinHttpHandler for .NET Framework support?

Do you plan on supporting WinHttpHandler for .NET Framework support?

No

JamesNK · 2024-08-06T13:19:56Z

...ry.Exporter.OpenTelemetryProtocol/Implementation/Custom/ExportClient/OtlpGrpcExportClient.cs

+            // We do not need to return back response and deadline for successful response so using cached value.
+            return SuccessExportResponse;
+        }
+        catch (Exception ex)


This whole catch block is a bit of a mess. Why not separate catch statements?

JamesNK · 2024-08-06T13:21:31Z

...ry.Exporter.OpenTelemetryProtocol/Implementation/Custom/ExportClient/OtlpGrpcExportClient.cs

+                // https://github.com/grpc/grpc-dotnet/blob/1416340c85bb5925b5fed0c101e7e6de71e367e0/src/Grpc.Net.Client/Internal/GrpcCall.cs#L799-L803
+                // Utilizing the inner exception here to determine deadline exceeded related failures.
+                // https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.sendasync?view=net-8.0#remarks
+                if (ex.InnerException is TimeoutException)


Using HttpClient.Timeout for deadline is probably ok. I assume you have global deadline duration that is applied to all requests so timetout works.

For Grpc.Net.Client, the deadline can differ per call.

JamesNK · 2024-08-06T13:23:47Z

...ry.Exporter.OpenTelemetryProtocol/Implementation/Custom/ExportClient/OtlpGrpcExportClient.cs

+        var dataLength = contentLength - 5;
+        BinaryPrimitives.WriteUInt32BigEndian(data, (uint)dataLength);
+
+        // TODO: Support compression.


Do you ever send compressed gRPC request data today? If you don't, then it's not needed.

You probably need to handle compressed responses. There might be an OTLP gRPC server that sends a compressed response for some reason.

My understanding is users can specify grpc-encoding header which will result in compressed payload to be transmitted.

JamesNK · 2024-08-06T13:25:43Z

...ry.Exporter.OpenTelemetryProtocol/Implementation/Custom/ExportClient/OtlpGrpcExportClient.cs

+        // grpc-dotnet calls specifies the HttpCompletion.ResponseHeadersRead.
+        // However, it is useful specifically for streaming calls?
+        // https://github.com/grpc/grpc-dotnet/blob/1416340c85bb5925b5fed0c101e7e6de71e367e0/src/Grpc.Net.Client/Internal/GrpcCall.cs#L485-L486
+        return this.HttpClient.SendAsync(request, cancellationToken).GetAwaiter().GetResult();


This is blocking so you need to worry about thread pool starvation. Are OTLP gRPC calls made on a dedicated thread? If so, that means a maximum of one thread is blocked and this isn't a bad.

Are OTLP gRPC calls made on a dedicated thread

Yes. (but there will be 3 such threads created by traces,logs,metrics)

JamesNK · 2024-08-06T13:27:15Z

....Exporter.OpenTelemetryProtocol/Implementation/Custom/ExportClient/TrailingHeadersHelpers.cs

+    public static readonly string ResponseTrailersKey = "__ResponseTrailers";
+
+    public static HttpHeaders TrailingHeaders(this HttpResponseMessage responseMessage)
+    {
+#if !NETSTANDARD2_0 && !NET462
+        return responseMessage.TrailingHeaders;
+#else
+        if (responseMessage.RequestMessage.Properties.TryGetValue(ResponseTrailersKey, out var headers) &&
+            headers is HttpHeaders httpHeaders)
+        {
+            return httpHeaders;
+        }
+
+        // App targets .NET Standard 2.0 and the handler hasn't set trailers
+        // in RequestMessage.Properties with known key. Return empty collection.
+        // Client call will likely fail because it is unable to get a grpc-status.
+        return ResponseTrailers.Empty;
+#endif
+    }


Do you plan to support .NET Framework with WinHttpHandler? If not, then this can be simplified to always return TrailingHeaders.

github-actions · 2024-08-14T03:17:44Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

github-actions · 2024-08-22T03:17:40Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

vishweshbankwar added 30 commits June 17, 2024 13:19

draft - all unit test passing

ed56da2

rename traceserializer

14414a8

Refactor 1

59871e5

refactor + http benchmark

4e3f137

ActivitySizeCalculator refactor

4025fe2

refactor - ComputeSizeWithTagAndLengthPrefix

cf28621

refactor + access modifier changes

034cc77

refactor + re-arrange

8e7b9dd

Refactor - Separate common types

96efcea

benchmarks + mock collector tests

e0ff831

rename

76f7257

refactor

2f64916

TODO

64218f8

refactor

2f7a95a

test update

c889b49

test

01b4686

ref

c410d90

grpc - handle additional exceptions

4133155

refactor

4e29ef0

grpc updates

c7784b7

code comment

99e3721

re-structure

5364244

clean up dup

0794fa8

grpc persistent storage

59251ac

temp - remove custom length part

f19e1e5

Merge branch 'main' into vibankwa/otlp-remove-trace-google-protobuf-d…

5b3f8a0

…ependency

clean up

7fa55b6

grpc license

2b753a2

Merge branch 'main' into vibankwa/otlp-remove-trace-google-protobuf-d…

940b19d

…ependency

nullable

f1e9aab

vishweshbankwar added 2 commits July 1, 2024 08:56

Alternate approach

04a90ec

grpc update

2b4756c

github-actions bot added the pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package label Jul 1, 2024

vishweshbankwar commented Jul 1, 2024

View reviewed changes

vishweshbankwar requested a review from JamesNK July 1, 2024 16:47

vishweshbankwar mentioned this pull request Jul 1, 2024

Reuse dictionaries in ActivityExtensions, MetricItemExtensions and OtlpLogExporter #5727

Closed

4 tasks

vishweshbankwar changed the title ~~[Do not Merge] Remove Google.Protobuf and Grpc.Net.Client dependency~~ [Do not Merge] [Otlp Exporter] Remove Google.Protobuf and Grpc.Net.Client dependency Jul 1, 2024

vishweshbankwar mentioned this pull request Jul 2, 2024

Many logs from OtlpMetricExporter #5717

Open

Kielek reviewed Jul 3, 2024

View reviewed changes

Merge branch 'main' into vibankwa/otlp-remove-protobuf-dependency-poc

62f30f0

rajkumar-rangaraj reviewed Jul 8, 2024

View reviewed changes

Tweaks.

3b10436

CodeBlanch reviewed Jul 8, 2024

View reviewed changes

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jul 16, 2024

github-actions bot closed this Jul 23, 2024

vishweshbankwar reopened this Jul 31, 2024

github-actions bot added perf Performance related and removed Stale Issues and pull requests which have been flagged for closing due to inactivity labels Jul 31, 2024

JamesNK reviewed Aug 6, 2024

View reviewed changes

Merge branch 'main' into vibankwa/otlp-remove-protobuf-dependency-poc

5e78598

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Aug 14, 2024

vishweshbankwar mentioned this pull request Aug 14, 2024

Adding OpenTelemetry to a native AOT app adds a lot of app size #5785

Open

github-actions bot closed this Aug 22, 2024

Kielek mentioned this pull request Sep 17, 2024

[OTLP exporter] Remove dependency on deprecated gRPC library #4860

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Do not Merge] [Otlp Exporter] Remove Google.Protobuf and Grpc.Net.Client dependency #5731

[Do not Merge] [Otlp Exporter] Remove Google.Protobuf and Grpc.Net.Client dependency #5731

vishweshbankwar commented Jul 1, 2024 •

edited

Loading

vishweshbankwar Jul 1, 2024

codecov bot commented Jul 1, 2024 •

edited

Loading

Kielek Jul 3, 2024

CodeBlanch Jul 8, 2024

rajkumar-rangaraj commented Jul 8, 2024

rajkumar-rangaraj Jul 8, 2024

CodeBlanch Jul 8, 2024

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 23, 2024

JamesNK Aug 6, 2024

vishweshbankwar Aug 6, 2024

JamesNK Aug 6, 2024

JamesNK Aug 6, 2024

JamesNK Aug 6, 2024

vishweshbankwar Aug 6, 2024

JamesNK Aug 6, 2024

cijothomas Aug 6, 2024 •

edited

Loading

JamesNK Aug 6, 2024

github-actions bot commented Aug 14, 2024

github-actions bot commented Aug 22, 2024

[Do not Merge] [Otlp Exporter] Remove Google.Protobuf and Grpc.Net.Client dependency #5731

[Do not Merge] [Otlp Exporter] Remove Google.Protobuf and Grpc.Net.Client dependency #5731

Conversation

vishweshbankwar commented Jul 1, 2024 • edited Loading

Changes

Merge requirement checklist

Choose a reason for hiding this comment

codecov bot commented Jul 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajkumar-rangaraj commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 14, 2024

github-actions bot commented Aug 22, 2024

vishweshbankwar commented Jul 1, 2024 •

edited

Loading

codecov bot commented Jul 1, 2024 •

edited

Loading

cijothomas Aug 6, 2024 •

edited

Loading