Prometheus: Expand UseOpenTelemetryPrometheusScrapingEndpoint extension #3029
Conversation
…exible and support more advanced scenarios.
|
So... the challenge with something like this is that it feels really weird to have some of the connection/endpoint settings in You also end up with the potential shoot-yourself-in-the-foot setup of: app.UseOpenTelemetryPrometheusScrapingEndpoint(
predicate: ctx =>
ctx.Connection.LocalPort == internalPort && ctx.Request.Path == "/not-the-right-path");...which then means, again, you run into a weird sort of double-location where you can control the endpoint. I actually mentioned this option in my original PR comment a bit because the only way you can trust that the predicate is in control is to also ignore the endpoint configuration in the options. |
|
@tillig What I think we should do is provide an easy way to get up and running that would satisfy the most common use case. The To add Prometheus in the default ASP.NET Core .NET 6 template we currently have this: using OpenTelemetry.Metrics;
var builder = WebApplication.CreateBuilder(args);
// Add services to the container.
builder.Services.AddRazorPages();
builder.Services.AddOpenTelemetryMetrics(o => o.AddPrometheusExporter());
var app = builder.Build();
// Configure the HTTP request pipeline.
if (!app.Environment.IsDevelopment())
{
app.UseExceptionHandler("/Error");
// The default HSTS value is 30 days. You may want to change this for production scenarios, see https://aka.ms/aspnetcore-hsts.
app.UseHsts();
}
app.UseHttpsRedirection();
app.UseStaticFiles();
app.UseRouting();
app.UseAuthorization();
app.MapRazorPages();
app.UseOpenTelemetryPrometheusScrapingEndpoint();
app.Run();Not too bad! I think for "getting started" that is very nice and concise. One of the reasons why we have I'm guessing the next most common thing users will want to do is change the default path. Right now they can do: builder.Services.Configure<PrometheusExporterOptions>(o => o.ScrapeEndpointPath = "/admin/metrics");Or to bind it to configuration: services.Configure<PrometheusExporterOptions>(this.Configuration.GetSection("Prometheus"));That second case, where we bind to configuration, is highly compelling! I know folks doing environment variable injection into containers like:
If we don't have the options object and path property, we lose that. I just updated this PR so you can also now do the path configuration as a one-liner like this: app.UseOpenTelemetryPrometheusScrapingEndpoint(configure: o => o.ScrapeEndpointPath = "/otel")All that is left is the super advanced case. Changing ports. Or looking for custom headers. Perhaps having allow-lists by IP address, stuff like that. We could allow the public static IApplicationBuilder UseOpenTelemetryPrometheusScrapingEndpoint(
this IApplicationBuilder app,
MeterProvider meterProvider = null,
Func<HttpContext, bool> predicate = null,
Action<PrometheusExporterOptions> configure = null,
Action<IApplicationBuilder> configureBranchedPipeline = null)
{
if (predicate == null)
{
var options = app.ApplicationServices.GetOptions<PrometheusExporterOptions>();
configure?.Invoke(options);
string path = options.ScrapeEndpointPath ?? PrometheusExporterOptions.DefaultScrapeEndpointPath;
if (!path.StartsWith("/"))
{
path = $"/{path}";
}
predicate = context => context.Request.Path == path;
}
return app.MapWhen(
predicate,
builder =>
{
configureBranchedPipeline?.Invoke(builder);
builder.UseMiddleware<PrometheusExporterMiddleware>(meterProvider ?? app.ApplicationServices.GetRequiredService<MeterProvider>());
});
}I think some users might have the opposite reaction to that and wonder why |
Codecov Report
@@ Coverage Diff @@
## main #3029 +/- ##
==========================================
+ Coverage 84.78% 84.82% +0.04%
==========================================
Files 259 259
Lines 9294 9313 +19
==========================================
+ Hits 7880 7900 +20
+ Misses 1414 1413 -1
|
|
I think the simple case looks easy but you're probably incurring a lot of longer term pain by not separating the concerns of the routing and the exporter configuration up front. For example, with that You could still get that nice, clean startup case if the path was part of the Like you could have public static IApplicationBuilder UseOpenTelemetryPrometheusScrapingEndpoint(
this IApplicationBuilder app,
MeterProvider meterProvider = null,
string path = "/metrics")
{
// Do app.Map here, no need for options.
}
public static IApplicationBuilder UseOpenTelemetryPrometheusScrapingEndpoint(
this IApplicationBuilder app,
Func<HttpContext, bool> predicate,
MeterProvider meterProvider = null)
{
// Do app.MapWhen with the required predicate here, again no need for options.
}But I suppose as long as I can do what I need to do I'm good. I will admit I'll likely end up creating something like this to enforce the separation of concerns on our end, like a bridge set of options. public class SimplePrometheusOptions
{
public int ScrapeResponseCacheDurationMilliseconds { get; set; }
}
public class PrometheusConfigurator : IConfigureOptions<PrometheusExporterOptions>
{
private IOptions<SimplePrometheusOptions> _simple;
public PrometheusConfigurator(IOptions<SimplePrometheusOptions> simple)
{
this._simple = simple;
}
public void Configure(PrometheusExporterOptions options)
{
options.StartHttpListener = false;
options.ScrapeResponseCacheDurationMilliseconds = this._simple.ScrapeResponseCacheDurationMilliseconds;;
}
}
public static class ServiceCollectionExtensions
{
public static OptionsBuilder<SimplePrometheusOptions> AddPrometheusOptions(this IServiceCollection services)
{
// Add the default options
services.AddOptions<PrometheusExporterOptions>();
// Registering the configurator will automatically run when IOptions<PrometheusExporterOptions> is resolved
services.AddTransient<IConfigureOptions<PrometheusExporterOptions>, PrometheusConfigurator>();
// Allow more configuration
return services.AddOptions<SimplePrometheusOptions>();
}
}The idea there is I will mask the invalid options from the consumer. Re: configuration, since environment variables are already part of configuration, I'm also having to write the ability to specify defaults in JSON with overrides using the standard environment variable names. Doing a bind from JSON/config to an object model isn't so easy to figure out if overrides are happening in the environment, and it makes things hard if you're using something like a central config service that defines things using the standard SDK keys. public class JaegerExporterConfigurator : IConfigureOptions<JaegerExporterOptions>
{
private readonly IConfiguration _configuration;
public JaegerExporterConfigurator(IConfiguration configuration)
{
this._configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
}
public void Configure(JaegerExporterOptions options)
{
if (options == null)
{
throw new ArgumentNullException(nameof(options));
}
// config.TryGetValue<T> is an extension method that checks to see if a given section exists
// and only binds the corresponding value if it does. Config can have JSON, Environment, etc.
// all in one big hierarchical set. Means we can also use things like Azure App Configuration
// to centrally set endpoints for tracing.
//
// I also didn't find any public constants for the environment variables defined in the SDK so
// I had to add that.
//
// All of this is effectively the same as what's in the JaegerExporterOptions constructor but
// with no ties to config.
if (this._configuration.TryGetValue<string>(OpenTelemetryEnvironmentVariable.JaegerExporterProtocol, out var protocol))
{
// I moved this out to a separate class for testability and because it's internal, so I got to copy/paste it.
options.Protocol = JaegerExportProtocolMap.Map(protocol);
}
if (this._configuration.TryGetValue<string>(OpenTelemetryEnvironmentVariable.JaegerExporterAgentHost, out var agentHost))
{
options.AgentHost = agentHost;
}
if (this._configuration.TryGetValue<int>(OpenTelemetryEnvironmentVariable.JaegerExporterAgentPort, out var agentPort))
{
options.AgentPort = agentPort;
}
if (this._configuration.TryGetValue<Uri>(OpenTelemetryEnvironmentVariable.JaegerExporterEndpoint, out var endpoint))
{
if (!endpoint.IsAbsoluteUri)
{
throw new InvalidOperationException($"Jaeger endpoint must be expressed as an absolute URI. Value '{endpoint}' is not absolute.");
}
options.Endpoint = endpoint;
}
}
}Anyway, that's a tangent. On the Prometheus middleware... like I said, if I can do what I need to do, I'll be good, and I'll hide the extra stuff as needed. |
…apingEndpoint to smooth out different ways to specify options.
|
@tillig Thanks for all the help on this!
Something I did consider. Looking at the
|
|
I think with this new set of overloads I'll be able to do what I need to do. It still seems like a lot of work to go and own/support long-term to so you can avoid providing a simple You might want to consider having that final extension that takes all the parameters be something that doesn't use default parameter values and instead offer an explicit 0-parameter extension. You could get into a situation later where you need to add more extensions but have a weird "ambiguous match" problem due to something being able to match against the new overload and the one with a bunch of optionals. Re:
and
These are kind of tied together. I think that the "bind options from config" use case is really uninteresting. Actually, let me be more specific:
There's a big difference here. I'll start in reverse order. Why isn't binding interesting?Binding from configuration (so we're talking about the same thing) is when you have a configuration structure (which could be expressed in INI, environment variables, JSON, XML, or any of the other supported configuration formats - not just JSON) where you can call The key takeaway (TLDR) here is that the configuration format is a contract provided by the library, so it is likely better to explicitly determine that contract than it is to tie it to an object from code. First, as we're talking about potentially splitting the options into something where Prometheus might be two different sets of options - one for the exporter, one for the endpoint information (either middleware or standalone listener) - changing that in code is pretty easy and the compiler will catch issues, no problem. I take an upgrade of the library, recompile, good to go. Making sure I get that changed in config is not as easy to detect. Especially if I was using the configuration binding thing and not configuring anything, I may not get any sort of compiler error at all, but suddenly my Prometheus listener isn't on the place I configured anymore... because an object model changed. You can't change any internals such that the same config settings are read/managed by different components, it's now tied together forever. Next, if any config property is an Let's say you have a config setup like: {
"endpoints": ["first", "second"]
}Don't forget everything in JSON also needs to be able to be represented in INI, Environment Variables, etc. So what this really looks like is: {
"endpoints:0": "first",
"endpoints:1": "second"
}That way you can specify an environment variable override: That makes any sort of enumerable property (like The part that ties into the Jaeger thing is the big one for me, which comes to... Why is reading from config interesting?It's easiest to explain this in a specific use case. In the OpenTelemetry SDK, there's a set of well-known environment variables that configure things like the Jaeger exporter or OpenTelemetry exporter. (I know you already know that, but for the sake of completeness I'm mentioning it anyway.) Let's say you want to configure the Jaeger agent host for a service. Per the SDK, I should set I want to provide a default value for that which ships with my application, as part of Ideally what I could do is provide an {
"OTEL_EXPORTER_JAEGER_AGENT_HOST": "dev-agent"
}and then I could easily override that with an environment variable at deploy: That's pretty easy to visualize and there's only one config setting that has the agent in it. I could even do something in testing like: ...because the command line config provider works like that. Everything lines up. It's all the same name, whether it's JSON, INI, Environment Variables, command line args, whatever. However, that's not really how it works today. That stuff doesn't line up. Instead, there's both this object binding thing, where I could configure it like: {
"Jaeger": {
"AgentHost": "localhost",
}
}...and the environment variable. But the environment variable doesn't override config. If I want to override it, I can try to use the environment variable Thus, reading from config is super interesting. If done right, you can define a config contract that allows you to:
A great example of this in .NET Core is the Logging system. The configuration format was thought through such that they can refactor the logging internals, the stuff that parses the configuration, and change things as needed; but it's also easy to define and override without dealing in ordinal collections. Now, granted, it means more work to read and parse configuration. That does suck. But it's probably worth it for the reasons mentioned above. It also means the configuration mechanism is not as pretty as having a nice hierarchical object in JSON like there is now. However, the OpenTelemetry SDK sort of gave you the config contract you're stuck with, so... there's that. Plus, there's a use case where folks may have a standard template for deploying microservices of all types - .NET, Node, Ruby, whatever - and that template might provide the environment variable for the agent host. That should "just work" but if someone in .NET accidentally checks in a JSON file for testing that has You could do both...If you really wanted, you could probably have the object binding and the environment variables thing, but you'd have to make sure the environment variables (or configuration that uses the SDK environment variable names) takes precedence over the object binding every time. And that could be weird because you might have Anyway, I hope that helps clarify why I've got this var config = new ConfigurationBuilder().AddEnvironmentVariables().Build();It doesn't hurt that, by doing it that way, you get the added side benefit of all your config reading mechanisms being totally testable without having to actually change the environment variables on the process. |
|
@tillig I'm not opposed to making the middleware public. My feeling is that having extensions for the common cases is nice for users and sort of guides them to success. Want to get some feedback from the other maintainers.
Makes sense, I'll change it. RE: Options Thanks for the feedback! That all makes sense. I have run into the enumerable binding thing in |
|
My thoughts on what should apply when:
On that latter one, I may build config like: new ConfigurationBuilder()
.AddJsonFile("appsettings.json")
.AddJsonFile($"appsettings.{env}.json", optional: true)
.AddEnvironmentVariables()
.AddAzureAppConfiguration()
.Build()... and the precedence order is controlled by me. For the vast, vast majority of folks using the default
You wouldn't have to worry about precedence. It'd be taken care of for you. For ease of use, you COULD offer more factory-related methods to build things instead of having it part of the constructor. // Do what is in the constructor right now, except maybe use the config mechanism instead.
var opt = JaegerExporterOptions.FromEnvironment();
// Do the reading like I showed in IOptions<T>.
var opt = JaegerExporterOptions.FromConfiguration(config);And for convenience, you could have an I think that does affect the |
|
(Converted to a draft. I'll switch it back to live PR when I get tests added.) |
|
Did some testing with this offline. I was able to configure the middleware on a custom port with a different auth scheme (JWT) from the rest of the app (cookies). I think the extensions provide a good level of configurability. @alanwest @cijothomas Ready for review on this. Set up for my testing: using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Authentication.Cookies;
using Microsoft.AspNetCore.Authentication.JwtBearer;
using OpenTelemetry.Metrics;
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddRazorPages();
builder.Services.AddOpenTelemetryMetrics(builder =>
builder
.AddAspNetCoreInstrumentation()
.AddPrometheusExporter());
builder.Services
.AddAuthentication(CookieAuthenticationDefaults.AuthenticationScheme) // Default policy always uses cookies.
.AddCookie() // Add cookie auth scheme for normal calls.
.AddJwtBearer(); // Add JWT auth scheme for internal calls.
var app = builder.Build();
if (!app.Environment.IsDevelopment())
{
app.UseExceptionHandler("/Error");
}
app.UseOpenTelemetryPrometheusScrapingEndpoint(
meterProvider: null,
predicate: context => context.Request.Path == "/internal/metrics" && context.Connection.LocalPort == 5067,
path: null,
configureBranchedPipeline: app =>
{
// This could be made into a middleware itself, just doing inline for demo purposes.
app.Use(async (httpcontext, next) =>
{
// Execute auth pipeline for JWT scheme.
var result = await httpcontext.AuthenticateAsync(JwtBearerDefaults.AuthenticationScheme).ConfigureAwait(false);
// Require JWT auth before allowing Prometheus.
if (!result.Succeeded || result.Principal?.Identity?.IsAuthenticated != true)
{
httpcontext.Response.StatusCode = 401;
return;
}
await next().ConfigureAwait(false); // Flow through to Prometheus.
});
});
app.UseStaticFiles();
app.UseAuthentication();
app.UseRouting();
app.UseAuthorization(); // Allow [Authorize] on razor pages which will use cookies.
app.MapRazorPages();
app.Run(); |
|
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. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
Ping @cijothomas @alanwest |
src/OpenTelemetry.Exporter.Prometheus/PrometheusExporterApplicationBuilderExtensions.cs
Show resolved
Hide resolved
| return RunPrometheusExporterMiddlewareIntegrationTest( | ||
| "/metrics", | ||
| app => app.UseOpenTelemetryPrometheusScrapingEndpoint(), | ||
| services => services.Configure<PrometheusExporterOptions>(o => o.ScrapeEndpointPath = null)); |
There was a problem hiding this comment.
Seems unlikely that someone would set this to null and expect the path to result in /metrics. Should we fail initialization in this case?
There was a problem hiding this comment.
Oh, is this the case where options are read from configuration - like a file, environment, etc? If so, does null in this context effectively mean the user didn't specify it and that the default would be expected?
There was a problem hiding this comment.
@alanwest What this test is verifying is that if the user does mess up and specify null (either from code or through config binding) for the endpoint path we fallback to the default path. I could switch it to a throw (fast-fail) on startup for that case. Might be a better experience for users that way 🤔
| /// <param name="predicate">Optional predicate for deciding if a given | ||
| /// <see cref="HttpContext"/> should be branched. If supplied <paramref | ||
| /// name="path"/> is ignored.</param> | ||
| /// <param name="path">Optional path to use for the branched pipeline. | ||
| /// Ignored if <paramref name="predicate"/> is supplied.</param> |
There was a problem hiding this comment.
I think I'd kinda prefer separate overloads... that is, one that takes a predicate and another that take a path.
I imagine you were trying to keep the overload permutations to a minimum, but passing in either null or a value that is ignored feels funny.
Side question... I've pondered when if ever we should consider adopting nullable reference types.
There was a problem hiding this comment.
mm... we can explore this further and decide if we want to add more overloads, to make usage looks less awkward.
There was a problem hiding this comment.
I imagine you were trying to keep the overload permutations to a minimum, but passing in either null or a value that is ignored feels funny.
For the overload with everything specified I tried to capture all the behaviors in the XML comments but ya, don't totally love this. Went through a few iterations of different numbers of extensions, different optional parameters, etc. Nothing felt totally 100% solid.
Side question... I've pondered when if ever we should consider adopting nullable reference types.
We should yes! I tried it once in a branch, lots and lots of effort to pull it off 😭 You can turn on nullable analysis per file, so that part is nice for rolling it out over time. But what I couldn't figure out was how to tell public api analyzer how to respect the per-file mode. It seemed like an all or nothing thing where we would have to do it all in one shot per project. Painful that way!
| /// <paramref name="path"/>.</remarks> | ||
| /// <param name="app">The <see cref="IApplicationBuilder"/> to add | ||
| /// middleware to.</param> | ||
| /// <param name="path">Path to use for the branched pipeline.</param> |
There was a problem hiding this comment.
we fallback to ScrapeEndpointPath, if path is not given, right?
There was a problem hiding this comment.
- In the case of the extension which accepts path, you are required to specify it:
public static IApplicationBuilder UseOpenTelemetryPrometheusScrapingEndpoint(this IApplicationBuilder app, string path)
{
Guard.ThrowIfNull(path);I figured if user was calling the path extension, without path, they probably made a mistake!
- In the case of the extension where you can specify everything, there is fallback logic:
if (predicate == null) // Respect predicate if set
{
if (path == null) // Respect path if set
{
var options = app.ApplicationServices.GetOptions<PrometheusExporterOptions>();
// Respect options if set otherwise fallback to default
path = options.ScrapeEndpointPath ?? PrometheusExporterOptions.DefaultScrapeEndpointPath;
}
|
@tillig Could you confirm if this unblocks your scenarios? |
|
@cijothomas Confirmed - I think I can work with this. Thanks for the efforts here! |
|
@CodeBlanch Please fix conflict, and we can merge this. (We can make this part of the 1.3.0.alpha.1 release, as 1.2.0 will not include Prometheus) |
…CodeBlanch/opentelemetry-dotnet into prometheus-middleware-extension
|
@cijothomas Merge conflict resolved. |
|
@tillig We did it! 🤣 |
|
WOOOO 🎉 🍾 |
|
Thanks again for this one, I have it integrated and it's working great! |
Fixes #2776
Alternative to #2969
Changes
This PR seeks to make the Prometheus middleware registration more flexible.
ScrapeEndpointPathas part of theAddPrometheusExportercall (a laoptions.AddPrometheusExporter(o => o.ScrapeEndpointPath = "/custom/path")) the middleware would not see that, it will now.Public API Changes
public static class PrometheusExporterApplicationBuilderExtensions { - public static IApplicationBuilder UseOpenTelemetryPrometheusScrapingEndpoint(this IApplicationBuilder app, MeterProvider meterProvider = null) + public static IApplicationBuilder UseOpenTelemetryPrometheusScrapingEndpoint(this IApplicationBuilder app) + public static IApplicationBuilder UseOpenTelemetryPrometheusScrapingEndpoint(this IApplicationBuilder app, string path) + public static IApplicationBuilder UseOpenTelemetryPrometheusScrapingEndpoint(this IApplicationBuilder app, Func<HttpContext, bool> predicate) + public static IApplicationBuilder UseOpenTelemetryPrometheusScrapingEndpoint(this IApplicationBuilder app, MeterProvider meterProvider, Func<HttpContext, bool> predicate, string path, Action<IApplicationBuilder> configureBranchedPipeline) }TODOs
CHANGELOG.mdupdated for non-trivial changes