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

Make OtlpExporterOptions.HttpClientFactory work with GRPC protocol #4625

Closed
wants to merge 1 commit into from

Conversation

dhhoang
Copy link

@dhhoang dhhoang commented Jun 29, 2023

Fixes #2120, #2009
Design discussion issue #3393

Changes

The purpose is to make it possible to control how HttpClient is created when using GRPC protocol. A common use case is to submit a client certificate, for example, to use with mTLS. The changes include:

  • Makes OtlpExporterOptions.HttpClientFactory option work with OtlpExportProtocol.Grpc protocol, instead of just OtlpExportProtocol.HttpProtobuf.
  • The HttpClient instance is created once when the channel is created, and it is passed to GrpcChannelOptions.
  • In addition, the use of IHttpClientFactory is also supported for GRPC in much the same way it is for HttpProtobuf.

Usage

The following code shows how to setup client certificate with this option enabled.

Services.AddOpenTelemetry().WithTracing(b =>
{
  b.AddOtlpExporter(expoterOpts=>{
    expoterOpts.Protocol = OtlpExportProtocol.Grpc;
    expoterOpts.HttpClientFactory = CreateOTLPHttpClient;
  });
});

private static HttpClient CreateOTLPHttpClient()
{
  var handler = new HttpClientHandler
  {
    ClientCertificateOptions = ClientCertificateOption.Manual,
    // use this to disable peer validation
    ServerCertificateCustomValidationCallback =HttpClientHandler.DangerousAcceptAnyServerCertificateValidator
  };
  handler.ClientCertificates.Add(new X509Certificate2("mycert.pfx"));

  return new HttpClient(handler, true);
}

Merge requirement checklist

  • CONTRIBUTING guidelines followed (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) (N/A)

@dhhoang dhhoang requested a review from a team June 29, 2023 17:46
@dhhoang dhhoang marked this pull request as draft June 29, 2023 17:46
@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #4625 (3c421d2) into main (104cf32) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4625      +/-   ##
==========================================
+ Coverage   84.90%   84.93%   +0.02%     
==========================================
  Files         313      313              
  Lines       12609    12616       +7     
==========================================
+ Hits        10706    10715       +9     
+ Misses       1903     1901       -2     
Impacted Files Coverage Δ
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 96.36% <ø> (ø)
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs 97.02% <100.00%> (+0.22%) ⬆️

... and 2 files with indirect coverage changes

@vishweshbankwar
Copy link
Member

Thanks @dhhoang The proposed change looks reasonable to me, was going to send the same PR shortly 😆

Adding @alanwest @CodeBlanch @utpilla to share feedback on proposal before reviewing.

@alanwest
Copy link
Member

@dhhoang Supporting the HttpClientFactory option with the gRPC exporter is something we're considering.

Though, if your need is primarily with the lack of support for client certificates, I think I'd be interested if we first pursued the configuration options currently missing that are required by the spec: Certificate file, Client key file, and Client certificate file.

@dhhoang
Copy link
Author

dhhoang commented Jun 30, 2023

@dhhoang Supporting the HttpClientFactory option with the gRPC exporter is something we're considering.

Though, if your need is primarily with the lack of support for client certificates, I think I'd be interested if we first pursued the configuration options currently missing that are required by the spec: Certificate file, Client key file, and Client certificate file.

@alanwest I'm actually about to send another PR to enable these configs, since they're needed for mTLS to work with .NET Framework targets.
That being said the ability to control HttpClient would be useful for other use cases (proxies etc.).
I saw the issue mentioned in #4616 and have made a comment there.

@alanwest
Copy link
Member

I'm actually about to send another PR to enable these configs, since they're needed for mTLS to work with .NET Framework targets.

Sounds good! For now, I think we could definitely proceed with the addition of the missing config options.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2023

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing 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 Jul 8, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
3 participants