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] Remove the Google.Protobuf / Grpc packages, and replace the logs and metrics with the new implementation #6005

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

rajkumar-rangaraj
Copy link
Contributor

Fixes #5730
Design discussion issue #

Changes

Please provide a brief description of the changes here.

OpenTelemetry.Exporter.OpenTelemetryProtocol

  • Replaced the current logs and metrics implementation with the new one. Deleted all existing implementations and renamed the new implementation to the existing names.
  • Remove the following package references from the OTLP Exporter project
    • Google.Protobuf
    • Grpc
    • Grpc.Net.Client
    • Grpc.Tools
  • Deleted all proto files from the project

Tests / Benchmarks

  • Updated both tests and benchmarks project to include proto files and the following packages Google.Protobuf, Grpc, Grpc.Tools and Grpc.Net.Client.
  • Updated the tests.

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)

@rajkumar-rangaraj rajkumar-rangaraj requested a review from a team as a code owner November 28, 2024 21:21
@github-actions github-actions bot added pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package perf Performance related labels Nov 28, 2024
{
Guard.ThrowIfNull(options);
Guard.ThrowIfNull(httpClient);
Guard.ThrowIfNull(signalPath);

Uri exporterEndpoint = options.Endpoint.AppendPathIfNotPresent(signalPath);
Uri exporterEndpoint;
if (options.Protocol == OtlpExportProtocol.Grpc)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a logical condition here to honor the existing behavior.

Copy link
Member

@CodeBlanch CodeBlanch Dec 2, 2024

Choose a reason for hiding this comment

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

Curious how it got by tests before?

Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 73.80952% with 22 lines in your changes missing coverage. Please review.

Project coverage is 85.82%. Comparing base (84e6afb) to head (0599950).

Files with missing lines Patch % Lines
....Exporter.OpenTelemetryProtocol/OtlpLogExporter.cs 56.00% 11 Missing ⚠️
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs 52.17% 11 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6005      +/-   ##
==========================================
+ Coverage   85.15%   85.82%   +0.67%     
==========================================
  Files         272      257      -15     
  Lines       12420    11690     -730     
==========================================
- Hits        10576    10033     -543     
+ Misses       1844     1657     -187     
Flag Coverage Δ
unittests-Project-Experimental 85.81% <73.80%> (+0.66%) ⬆️
unittests-Project-Stable 85.80% <73.80%> (+0.66%) ⬆️

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

Files with missing lines Coverage Δ
...ol/Implementation/ExportClient/OtlpExportClient.cs 100.00% <100.00%> (ø)
...mplementation/ExportClient/OtlpGrpcExportClient.cs 66.66% <ø> (ø)
...mplementation/ExportClient/OtlpHttpExportClient.cs 100.00% <ø> (ø)
...yProtocol/Implementation/ExportClient/OtlpRetry.cs 85.71% <100.00%> (+1.84%) ⬆️
...tlpExporterPersistentStorageTransmissionHandler.cs 87.27% <100.00%> (+52.49%) ⬆️
...ansmission/OtlpExporterRetryTransmissionHandler.cs 80.00% <100.00%> (+60.00%) ⬆️
...on/Transmission/OtlpExporterTransmissionHandler.cs 100.00% <100.00%> (+29.41%) ⬆️
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs 96.90% <100.00%> (+8.37%) ⬆️
...lemetryProtocol/OtlpLogExporterHelperExtensions.cs 94.26% <100.00%> (+0.67%) ⬆️
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs 96.51% <100.00%> (+1.05%) ⬆️
... and 3 more

... and 7 files with indirect coverage changes

@Kielek
Copy link
Contributor

Kielek commented Nov 29, 2024

🎉 I do not have enough time to review it right now, but it is great to see this progress.

@TimothyMothra
Copy link
Contributor

It looks like the proto files are copied into two directories, unit test & benchmarks.
Can these be stored in a common directory?


this.thread = new Thread(this.RetryStoredRequests)
{
Name = $"OtlpExporter Persistent Retry Storage - {typeof(TRequest)}",
Name = $"OtlpExporter Persistent Retry Storage",
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can remove the $

@github-actions github-actions bot added the documentation Documentation related label Dec 2, 2024
@@ -156,7 +156,7 @@ dotnet_diagnostic.IDE0005.severity = warning
# RS0041: Public members should not use oblivious types
dotnet_diagnostic.RS0041.severity = suggestion

[obj/**.cs]
[**/obj/**.cs]
Copy link
Member

Choose a reason for hiding this comment

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

@rajkumar-rangaraj This cleans up those proto warnings you had to suppress in project files.

@CodeBlanch CodeBlanch changed the title [otlp] Remove the Google.Protobuf / Grpc packages, and replace the logs and metrics with the new implementation. [otlp] Remove the Google.Protobuf / Grpc packages, and replace the logs and metrics with the new implementation Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation related perf Performance related pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom serializer for OTLP exporter
4 participants