feat(telemetry): core review fixes (Prometheus+OTLP coexistence, ServiceName validation, null guards) + contract overload note
- Fix #1: Prometheus exporter always wired for metrics; OTLP is additive overlay when Exporter == ZbExporter.Otlp so /metrics + MapZbMetrics work in all modes. - Fix #2: BuildOptions throws ArgumentException when ServiceName is null/whitespace. - Fix #3: AddZbTelemetry(IHostApplicationBuilder) guard: ThrowIfNull(configure) added alongside existing ThrowIfNull(builder). - Fix #6: Contract doc adds IServiceCollection convenience overload signature. - Tests: +3 new tests (OtlpExporter still serves /metrics, empty ServiceName throws, whitespace ServiceName throws). Total: 7 passed (was 4).
This commit is contained in:
@@ -25,6 +25,7 @@ public static class ZbTelemetryExtensions
|
||||
Action<ZbTelemetryOptions> configure)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(builder);
|
||||
ArgumentNullException.ThrowIfNull(configure);
|
||||
builder.Services.AddZbTelemetry(BuildOptions(configure));
|
||||
return builder;
|
||||
}
|
||||
@@ -94,20 +95,23 @@ public static class ZbTelemetryExtensions
|
||||
ArgumentNullException.ThrowIfNull(configure);
|
||||
var options = new ZbTelemetryOptions();
|
||||
configure(options);
|
||||
if (string.IsNullOrWhiteSpace(options.ServiceName))
|
||||
{
|
||||
throw new ArgumentException(
|
||||
"ZbTelemetryOptions.ServiceName is required (e.g. \"otopcua\").",
|
||||
nameof(configure));
|
||||
}
|
||||
return options;
|
||||
}
|
||||
|
||||
private static void ApplyMetricsExporter(MeterProviderBuilder metrics, ZbTelemetryOptions options)
|
||||
{
|
||||
switch (options.Exporter)
|
||||
// Prometheus is always wired so that /metrics and MapZbMetrics() work regardless of
|
||||
// the exporter setting. OTLP is an additive overlay when explicitly requested.
|
||||
metrics.AddPrometheusExporter();
|
||||
if (options.Exporter == ZbExporter.Otlp)
|
||||
{
|
||||
case ZbExporter.Otlp:
|
||||
metrics.AddOtlpExporter(o => ConfigureOtlp(o, options));
|
||||
break;
|
||||
case ZbExporter.Prometheus:
|
||||
default:
|
||||
metrics.AddPrometheusExporter();
|
||||
break;
|
||||
metrics.AddOtlpExporter(o => ConfigureOtlp(o, options));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
using System.Diagnostics.Metrics;
|
||||
using Microsoft.AspNetCore.Builder;
|
||||
using Microsoft.AspNetCore.Hosting;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using OpenTelemetry;
|
||||
using OpenTelemetry.Metrics;
|
||||
@@ -10,6 +11,66 @@ namespace ZB.MOM.WW.Telemetry.Tests;
|
||||
|
||||
public sealed class AddZbTelemetryTests
|
||||
{
|
||||
// Fix #2: empty ServiceName must throw ArgumentException --------------------------
|
||||
|
||||
[Fact]
|
||||
public void AddZbTelemetry_Throws_WhenServiceNameIsEmpty()
|
||||
{
|
||||
var builder = WebApplication.CreateBuilder();
|
||||
var ex = Assert.Throws<ArgumentException>(() =>
|
||||
builder.AddZbTelemetry(o =>
|
||||
{
|
||||
o.ServiceName = ""; // explicitly empty
|
||||
}));
|
||||
Assert.Equal("configure", ex.ParamName);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AddZbTelemetry_Throws_WhenServiceNameIsWhitespace()
|
||||
{
|
||||
var builder = WebApplication.CreateBuilder();
|
||||
var ex = Assert.Throws<ArgumentException>(() =>
|
||||
builder.AddZbTelemetry(o =>
|
||||
{
|
||||
o.ServiceName = " ";
|
||||
}));
|
||||
Assert.Equal("configure", ex.ParamName);
|
||||
}
|
||||
|
||||
// Fix #1: Prometheus coexists with OTLP — /metrics must still serve under Otlp exporter
|
||||
|
||||
[Fact]
|
||||
public async Task AddZbTelemetry_OtlpExporter_StillServesPrometheusEndpoint()
|
||||
{
|
||||
var builder = WebApplication.CreateBuilder();
|
||||
builder.WebHost.UseUrls("http://127.0.0.1:0");
|
||||
builder.AddZbTelemetry(o =>
|
||||
{
|
||||
o.ServiceName = "telemetry-test";
|
||||
o.Exporter = ZbExporter.Otlp;
|
||||
// OtlpEndpoint intentionally left null — exporter will be registered but won't
|
||||
// connect anywhere; we are only verifying Prometheus remains present.
|
||||
o.Meters = ["Test.OtlpCoexist.Meter"];
|
||||
});
|
||||
|
||||
await using var app = builder.Build();
|
||||
app.MapZbMetrics();
|
||||
|
||||
await app.StartAsync();
|
||||
|
||||
var address = app.Urls.First();
|
||||
using var client = new HttpClient { BaseAddress = new Uri(address) };
|
||||
|
||||
var response = await client.GetAsync("/metrics");
|
||||
|
||||
Assert.Equal(System.Net.HttpStatusCode.OK, response.StatusCode);
|
||||
Assert.Equal("text/plain", response.Content.Headers.ContentType?.MediaType);
|
||||
|
||||
await app.StopAsync();
|
||||
}
|
||||
|
||||
// Existing test ---------------------------------------------------------------
|
||||
|
||||
[Fact]
|
||||
public void AddZbTelemetry_ExportsAppMeter_WithSharedResource()
|
||||
{
|
||||
|
||||
@@ -111,6 +111,13 @@ public static class ZbTelemetryExtensions
|
||||
public static IServiceCollection AddZbTelemetry(
|
||||
this IServiceCollection services,
|
||||
ZbTelemetryOptions options);
|
||||
|
||||
/// IServiceCollection convenience overload that accepts a configure delegate.
|
||||
/// Equivalent to calling BuildOptions(configure) then AddZbTelemetry(services, options).
|
||||
/// Use when only an IServiceCollection is available but the lambda form is preferred.
|
||||
public static IServiceCollection AddZbTelemetry(
|
||||
this IServiceCollection services,
|
||||
Action<ZbTelemetryOptions> configure);
|
||||
}
|
||||
|
||||
/// Builds the shared OTel ResourceBuilder from ZbTelemetryOptions.
|
||||
|
||||
Reference in New Issue
Block a user