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

Use Grpc.Net.Client with netstandard2.0 for OpenTelemetry.Exporter.OpenTelemetryProtocol #3421

Closed
ismaelhamed opened this issue Jun 30, 2022 · 6 comments
Labels
enhancement New feature or request Stale Issues and pull requests which have been flagged for closing due to inactivity

Comments

@ismaelhamed
Copy link

Feature Request

Is your feature request related to a problem?

The old Grpc package copies libgrpc_csharp_ext.x64.dylib and libgrpc_csharp_ext.x86.so all over the place. That's ~17MB of unnecessary libs being copied to all of our the services that references OpenTelemetry.Exporter.OpenTelemetryProtocol in our solution.

Describe the solution you'd like:

It should be possible to use Grpc.Net.Client with netstandard2.0 instead. In fact, in theory, any net462 or greater app should be able to reference netstandard2.0.

@ismaelhamed ismaelhamed added the enhancement New feature or request label Jun 30, 2022
@cijothomas
Copy link
Member

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj#L10-L16

Looks like we already use the Grpc.Net.Client for netstandard2.1_or_higher. The package did not originally support netstandard2.0..

@alanwest explored this, in the context of Grpc instrumentation as well.

@alanwest
Copy link
Member

alanwest commented Jul 6, 2022

Agreed we should solve this. Apart from the old gRPC library coping unnecessary native libs all over the place, it's also on the path to deprecation https://grpc.io/blog/grpc-csharp-future/.

@alanwest explored this, in the context of Grpc instrumentation as well.

Yes, I added the netstandard2.0 target to the gRPC instrumentation with minimal test coverage - that is, the test coverage does not make end-to-end calls to a real gRPC service. I think this coverage was adequate in the context of the instrumentation since it is the end user's responsibility to manage the gRPC channel and make it work with respect to the configuration required for older frameworks/environments as noted here https://docs.microsoft.com/aspnet/core/grpc/netstandard#net-implementations.

Currently, our OTLP exporter's configuration does not allow the end user to fully manage the channel, so without having dug into things too deeply, I think there are three paths we could explore:

  1. Continue to manage the channel internal to the OTLP exporter. This has the benefit of maintaining a simple configuration surface area for our OTLP exporter. Though I think we would need to thoroughly test that the exporter still functions correctly in a wider variety of environments than we do today.

  2. Make it the end user's problem. That is, expose the ability for the end user to construct their own gRPC channel and pass it to the OTLP exporter. It would be up to the end user to understand how to correctly instantiate the channel based on their environment.

  3. It's possible that there's some overlap here with the request from Ability to use OTLP servers with self-signed certificates #3393. This option is similar to option 2, but instead of exposing the gRPC channel, it's possible that it would be enough to just use the HttpClientFactory configuration we already expose with the OTLP/gRPC exporter. Reading over https://docs.microsoft.com/aspnet/core/grpc/netstandard#net-implementations, it seems to suggest that it's the underlying HttpClient that needs to be configured correctly based on the environment.

Given that there are already some other use cases that may be satisfied by option 3, I'd be inclined to explore this first.

@CodeBlanch
Copy link
Member

Someone was asking about channel customization on Slack today so I messed with option 3 a bit. Seems like it could work:

        public static GrpcChannel CreateChannel(this OtlpExporterOptions options)
        {
            if (options.Endpoint.Scheme != Uri.UriSchemeHttp && options.Endpoint.Scheme != Uri.UriSchemeHttps)
            {
                throw new NotSupportedException($"Endpoint URI scheme ({options.Endpoint.Scheme}) is not supported. Currently only \"http\" and \"https\" are supported.");
            }

            return GrpcChannel.ForAddress(
                options.Endpoint,
                new GrpcChannelOptions
                {
                    HttpClient = options.HttpClientFactory?.Invoke() ?? throw new InvalidOperationException("OtlpExporterOptions was missing HttpClientFactory or it returned null."),
                    DisposeHttpClient = true,
                });
        }

Just documenting here what I tried. Leaving it up to @alanwest (our gRPC expert) what we should do here 😄

@Yun-Ting
Copy link
Contributor

I've chatted with @alanwest, and we are currently thinking to split the existing OTLP package into 2:

  1. OpenTelemetry.Exporter.OpenTelemetryProtocol.Grpc, and
  2. OpenTelemetry.Exporter.OpenTelemetryProtocol.Http

Please refer to the comment for more details:
#3885 (comment)

Copy link
Contributor

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

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 25, 2024
Copy link
Contributor

github-actions bot commented Oct 3, 2024

Closed as inactive. Feel free to reopen if this issue is still a concern.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
5 participants