Skip to content

Conversation

@rajkumar-rangaraj
Copy link
Member

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

Uri exporterEndpoint = options.Endpoint.AppendPathIfNotPresent(signalPath);
Uri exporterEndpoint;
if (options.Protocol == OtlpExportProtocol.Grpc)
Copy link
Member 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
Member Author

Choose a reason for hiding this comment

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

New implementation was never tested on it. On the replacement we caught this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

The actual issue was with the HTTP implementation. In the gRPC case, we append the path in all scenarios. However, for HTTP, the path is appended only if it is provided by default or through environment variables. If an endpoint is explicitly set, we do not append the path in the HTTP case, but this behavior was not being followed in new implementation.

During my initial implementation, I mistakenly assumed this was a bug and removed the check. However, there was an existing test case that validated this behavior.

exporterEndpoint = options.AppendSignalPathToEndpoint
                ? options.Endpoint.AppendPathIfNotPresent(signalPath)
                : options.Endpoint;

@codecov
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 86.23%. Comparing base (84e6afb) to head (c54a555).
Report is 1 commits behind head on main.

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%   86.23%   +1.08%     
==========================================
  Files         272      257      -15     
  Lines       12420    11690     -730     
==========================================
- Hits        10576    10081     -495     
+ Misses       1844     1609     -235     
Flag Coverage Δ
unittests-Project-Experimental 86.07% <73.80%> (+0.92%) ⬆️
unittests-Project-Stable 86.11% <73.80%> (+0.97%) ⬆️
unittests-Solution 86.13% <73.80%> (?)
unittests-UnstableCoreLibraries-Experimental 85.78% <ø> (?)

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 92.72% <100.00%> (+57.94%) ⬆️
...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 10 files with indirect coverage changes

@Kielek
Copy link
Member

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

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

@github-actions github-actions bot added the documentation Documentation related label Dec 2, 2024
@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
Comment on lines -26 to -31
<ItemGroup>
<PackageReference Include="Grpc.Net.Client" Condition="'$(TargetFramework)' != 'netstandard2.0' AND '$(TargetFramework)' != '$(NetFrameworkMinimumSupportedVersion)'" />
<PackageReference Include="Grpc" Condition="'$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == '$(NetFrameworkMinimumSupportedVersion)'" />
<PackageReference Include="Google.Protobuf" />
<PackageReference Include="Grpc.Tools" PrivateAssets="All" />
</ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

Dropping these dependencies is probably worthy of a CHANGELOG entry? What you think @alanwest?

Copy link
Member Author

Choose a reason for hiding this comment

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

This definitely needs to be a part of the changelog. There are few things we need to cover in the changelog, planning to cover as follow-up.

  1. Removal of this packages
  2. Breaking change on PeerServiceResolver, in one the earlier PR got feedback from you and Alan and we decided to remove this look up as we aren't aware why this logic was present https://github.com/open-telemetry/opentelemetry-dotnet/blob/84e6afbebae67f4e9b498f1702edfe4ceccf34ec/src/Shared/PeerServiceResolver.cs
  3. Call-out on the internal serialization change.

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

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