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.

Sign up for GitHub

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

Jump to bottom

[otlp] Remove the Google.Protobuf / Grpc packages, and replace the logs and metrics with the new implementation #6005

Merged
merged 10 commits into from
Dec 3, 2024
Merged

[otlp] Remove the Google.Protobuf / Grpc packages, and replace the logs and metrics with the new implementation #6005

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1f551af
Replace logs with new impl
rajkumar-rangaraj Nov 28, 2024
9aa77bc
Remove package reference, remove proto files, replace logs and metrics
rajkumar-rangaraj Nov 28, 2024
cb52d26
Update Benchmarks
rajkumar-rangaraj Nov 28, 2024
d7b7797
Test updates
rajkumar-rangaraj Nov 28, 2024
8a0beb9
Lint fixes
rajkumar-rangaraj Nov 28, 2024
0599950
Cleanup proto README.
CodeBlanch Dec 2, 2024
bf4becd
Tweaks.
CodeBlanch Dec 2, 2024
d07f706
Lint.
CodeBlanch Dec 2, 2024
41dc362
Clean up.
CodeBlanch Dec 2, 2024
c54a555
nits
rajkumar-rangaraj Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 0 additions & 67 deletions ...ry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/BaseOtlpGrpcExportClient.cs
View file
Open in desktop

This file was deleted.

110 changes: 0 additions & 110 deletions ...ry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/BaseOtlpHttpExportClient.cs
View file
Open in desktop

This file was deleted.

8 changes: 4 additions & 4 deletions ...OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/IExportClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@
namespace OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient;

/// <summary>Export client interface.</summary>
/// <typeparam name="TRequest">Type of export request.</typeparam>
internal interface IExportClient<in TRequest>
internal interface IExportClient
{
/// <summary>
/// Method for sending export request to the server.
/// </summary>
/// <param name="request">The request to send to the server.</param>
/// <param name="buffer">The request body to send to the server.</param>
/// <param name="contentLength">length of the content.</param>
/// <param name="deadlineUtc">The deadline time in utc for export request to finish.</param>
/// <param name="cancellationToken">An optional token for canceling the call.</param>
/// <returns><see cref="ExportClientResponse"/>.</returns>
ExportClientResponse SendExportRequest(TRequest request, DateTime deadlineUtc, CancellationToken cancellationToken = default);
ExportClientResponse SendExportRequest(byte[] buffer, int contentLength, DateTime deadlineUtc, CancellationToken cancellationToken = default);

/// <summary>
/// Method for shutting down the export client.
Expand Down
30 changes: 0 additions & 30 deletions ...metry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/IProtobufExportClient.cs
View file
Open in desktop

This file was deleted.

19 changes: 15 additions & 4 deletions .../ExportClient/ProtobufOtlpExportClient.cs → ...entation/ExportClient/OtlpExportClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@

namespace OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient;

internal abstract class ProtobufOtlpExportClient : IProtobufExportClient
internal abstract class OtlpExportClient : IExportClient
{
private static readonly Version Http2RequestVersion = new(2, 0);

#if NET
private static readonly bool SynchronousSendSupportedByCurrentPlatform;

static ProtobufOtlpExportClient()
static OtlpExportClient()
{
#if NET
// See: https://github.com/dotnet/runtime/blob/280f2a0c60ce0378b8db49adc0eecc463d00fe5d/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs#L767
Expand All @@ -28,13 +28,24 @@ static ProtobufOtlpExportClient()
}
#endif

protected ProtobufOtlpExportClient(OtlpExporterOptions options, HttpClient httpClient, string signalPath)
protected OtlpExportClient(OtlpExporterOptions options, HttpClient httpClient, string signalPath)
{
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 •
edited
Loading

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
Contributor 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
Contributor 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;

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

this.Endpoint = new UriBuilder(exporterEndpoint).Uri;
this.Headers = options.GetHeaders<Dictionary<string, string>>((d, k, v) => d.Add(k, v));
this.HttpClient = httpClient;
Expand Down
4 changes: 2 additions & 2 deletions ...ortClient/ProtobufOtlpGrpcExportClient.cs → ...tion/ExportClient/OtlpGrpcExportClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
namespace OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient;

/// <summary>Base class for sending OTLP export request over gRPC.</summary>
internal sealed class ProtobufOtlpGrpcExportClient : ProtobufOtlpExportClient
internal sealed class OtlpGrpcExportClient : OtlpExportClient
{
public const string GrpcStatusDetailsHeader = "grpc-status-details-bin";
private static readonly ExportClientHttpResponse SuccessExportResponse = new(success: true, deadlineUtc: default, response: null, exception: null);
Expand All @@ -24,7 +24,7 @@ private static readonly ExportClientGrpcResponse DefaultExceptionExportClientGrp
status: null,
grpcStatusDetailsHeader: null);

public ProtobufOtlpGrpcExportClient(OtlpExporterOptions options, HttpClient httpClient, string signalPath)
public OtlpGrpcExportClient(OtlpExporterOptions options, HttpClient httpClient, string signalPath)
: base(options, httpClient, signalPath)
{
}
Expand Down
45 changes: 0 additions & 45 deletions ...try.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/OtlpGrpcLogExportClient.cs
View file
Open in desktop

This file was deleted.

45 changes: 0 additions & 45 deletions ...Exporter.OpenTelemetryProtocol/Implementation/ExportClient/OtlpGrpcMetricsExportClient.cs
View file
Open in desktop

This file was deleted.

Loading
Loading