Add OpenTelemetry support on yarp container#2750
Conversation
src/Application/Extensions.cs
Outdated
| if (useOtlpExporter) | ||
| { | ||
|
|
||
| if (string.Equals(Environment.GetEnvironmentVariable("YARP_UNSAFE_SKIP_OLTP_CERT_VALIDATION"), "true", StringComparison.InvariantCultureIgnoreCase)) |
There was a problem hiding this comment.
Nit: If this is otherwise equivalent, can we change this to something like?
builder.Services.Configure<OtlpExporterOptions>(options =>
{
options.HttpClientFactory = () =>
{
var handler = new HttpClientHandler();
if (string.Equals(Environment.GetEnvironmentVariable("YARP_UNSAFE_SKIP_OLTP_CERT_VALIDATION"), "true", StringComparison.InvariantCultureIgnoreCase))
{
handler.ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator;
}
return new HttpClient(handler);
}
});
builder.Services.AddOpenTelemetry()
.WithLogging(logging => logging.AddOtlpExporter())
.WithMetrics(metrics => metrics.AddOtlpExporter())
.WithTracing(tracing => tracing.AddOtlpExporter());There was a problem hiding this comment.
My comment here was more so about potential subtle differenes in behavior between the two (is the handler configured the same?), not just DangerousAcceptAnyServerCertificateValidator.
Right now it's not obvious that the environment variable will only change the cert validation.
ab795c0 to
a7f38f3
Compare
6689f14 to
88f3500
Compare
src/Application/Extensions.cs
Outdated
| if (useOtlpExporter) | ||
| { | ||
|
|
||
| if (string.Equals(Environment.GetEnvironmentVariable("YARP_UNSAFE_SKIP_OLTP_CERT_VALIDATION"), "true", StringComparison.InvariantCultureIgnoreCase)) |
There was a problem hiding this comment.
My comment here was more so about potential subtle differenes in behavior between the two (is the handler configured the same?), not just DangerousAcceptAnyServerCertificateValidator.
Right now it's not obvious that the environment variable will only change the cert validation.
|
Would really love for this to get merged! |
Add opentelemetry support for the yarp container.
The main issue with the current solution is that when running in Aspire, the container will try to export telemetry to
host.docker.internal, which will trigger a SSL error, since the dev cert is only made forlocalhost.For the moment, I propose that we ignore this error when running in development.
Other option:
host.docker.internal, and manually trust the cert at runtime (requiring switching to a nondistrolessbase image)