-
Notifications
You must be signed in to change notification settings - Fork 772
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: use Grpc.Net.Client for netstandard2.1 #1662
OTLP exporter: use Grpc.Net.Client for netstandard2.1 #1662
Conversation
examples/Console/TestOtlpExporter.cs
Outdated
// Enable support for unencrypted HTTP2 | ||
AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required when calling insecure (i.e. http not https) gRPC service. This is no longer required for .NET 5.
I can document this in the readme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be good to have that note in the README.
// Enable OpenTelemetry for the sources "Samples.SampleServer" and "Samples.SampleClient" | ||
// and use OTLP exporter. | ||
using var openTelemetry = Sdk.CreateTracerProviderBuilder() | ||
.AddSource("Samples.SampleClient", "Samples.SampleServer") | ||
.SetResourceBuilder(ResourceBuilder.CreateDefault().AddService("otlp-test")) | ||
.AddOtlpExporter(opt => opt.Endpoint = endpoint) | ||
.AddOtlpExporter(opt => opt.Endpoint = new Uri(endpoint)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the netstandard2.1
build, I changed the Endpoint to be a Uri
instead of string
. This is because if the scheme is not present then the client throws:
System.ArgumentException: Only 'http' and 'https' schemes are allowed.
I figure using Uri
helps mitigate this.
/// <summary> | ||
/// Gets or sets the gRPC channel options. | ||
/// </summary> | ||
public GrpcChannelOptions GrpcChannelOptions { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GrpcChannelOptions
class in Grpc.Net.Client
should cover functionality previously covered with the Credentials
and ChannelOptions
configuration options.
Codecov Report
@@ Coverage Diff @@
## master #1662 +/- ##
==========================================
+ Coverage 81.80% 81.82% +0.01%
==========================================
Files 245 245
Lines 6607 6613 +6
==========================================
+ Hits 5405 5411 +6
Misses 1202 1202
|
We lack test coverage for sending data with a legit gRPC client - even prior to this PR. The existing unit tests all use this mock opentelemetry-dotnet/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterTest.cs Line 417 in 7a91923
I'd like to expand the coverage here, but haven't had a chance yet. I've manually tested the work in this PR pretty extensively. Depending on what folks would prefer, I can either expand the test coverage as part of this PR or in a follow up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Separately, we can modify the readme and examples to show both pre 3.0 and post 3.0 app code snippets, to help when people upgrade to newer .net versions?
Fixes #1651
Changes
This PR introduces a
netstandard2.1
build of the OTLP exporter. This enables the exporter to useGrpc.Net.Client
instead of the native gRPC library for .NET Core 3.0+ applications.We have had a number of issues with the native gRPC library that are resolved by using
Grpc.Net.Client
. Also,Grpc.Net.Client
is reportedly fast on .NET 5.This PR does introduce some breaking changes to the
OtlpExporterOptions
which needed to be adapted for use withGrpc.Net.Client
.For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes