Set PrometheusExporterMiddleware as public.#2969
Set PrometheusExporterMiddleware as public.#2969tillig wants to merge 5 commits intoopen-telemetry:mainfrom
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## main #2969 +/- ##
=======================================
Coverage 84.10% 84.10%
=======================================
Files 255 255
Lines 9063 9063
=======================================
Hits 7622 7622
Misses 1441 1441
|
|
Realized after the API compat build ran that I'd forgotten to update the There was an earlier failure on SQL Server timing out; I'm not sure what to do about that since I didn't actually change anything around SQL Server. |
|
This change will expose the traits that PrometheusExporter is built on top of (e.g. asp.net core, middleware). The challenge seems to be similar to #2882. I think we need to make a design decision - do we want PrometheusExporter to hide all the implementation traits or not? If we do need "a special PrometheusExporter that is ASP.NET Core friendly", do we want it to be the same exporter or a separate one? (e.g. what if there is another user saying "I want an OWIN or WCF friendly PrometheusExporter") |
|
My goal is to allow the ability to have tighter control over where/how the exporter is attached to my app pipeline. I'm falling into a gray area that isn't covered - it can't be on the same port as the rest of the services, but it also can't be a standalone listener, and those are the only options available at the moment. If you really wanted to hide it, you'd probably have to make some breaking API changes. For example, the Technically the actual Prometheus format response is generated by the still-internal Making the middleware itself public seemed to be a reasonable middle ground where it wouldn't introduce breaking changes, wouldn't add confusing/conflicting options, and would still allow folks like me to have that advanced ability to put the middleware in place in the pipeline with all the configuration we need. I mean, I'm totally open to other solutions. This just seemed to allow the most flexibility for consumers in the easiest, most non-breaking way possible. In the meantime, I'm having to do stuff like this to make it work: var configuredPath = app.ApplicationServices
.GetService<IOptions<PrometheusExporterOptions>>()
?.Value
?.ScrapeEndpointPath;
var path = new PathString(configuredPath ?? "/metrics");
bool Predicate(HttpContext c)
{
var success =
(port == null || c.Connection.LocalPort == port) &&
(!path.HasValue ||
(c.Request.Path.StartsWithSegments(path, out var remaining) &&
string.IsNullOrEmpty(remaining)));
return success;
}
var middlewareType =
typeof(PrometheusExporterOptions)
.Assembly.GetType("OpenTelemetry.Exporter.Prometheus.PrometheusExporterMiddleware");
if (middlewareType == null)
{
throw new InvalidOperationException("Unable to locate the Prometheus exporter middleware type.");
}
return app.MapWhen(
Predicate,
b => b.UseMiddleware(middlewareType, app.ApplicationServices.GetRequiredService<MeterProvider>()));That's because
|
Exactly! The PrometheusExporter is not yet Stable, and the spec is still Experimental so we should be able to take breaking changes.
Given that there were many "HTTP servers" and the space is still evolving, I don't think we will end up with "one fit for all" solution. I would suggest having a neutral PrometheusExporter which is agnostic to the underlying implementation, then having dedicated components (whether as a separate package, or some DI plugins) for concrete hosting (e.g. OWIN, ASP.NET Core). |
|
Oh, I see what you're saying - have, like, Of course, if that was added, I'd probably ask for the ability to expose the base exporter common bits so I could write my own exporter extension if needed. Like, right now the middleware "hides" the call to the I'd be totally OK with that if that's how the project wants to go. |
Exactly! And in the |
|
Cool. I guess, let me know if that's something to proceed with and/or if you'd like a PR for that instead of this. |
|
BTW, if you do want help there... I'm a MacOS + VS Code guy, so it'd be good to know if we could add the |
I had similar discussion with @CodeBlanch before, want to see if he has some ideas. |
I do see some other OpenTelemetry projects having I guess it's a choice of the maintainers if they would like to see this project associated with any IDEs or editors. I personally would slightly prefer the project to be neutral (IIRC I was questioning if we need the |
The CONTRIBUTING doc currently specifically lists "supported IDEs" so having the |
I'm convinced! 👍😄 |
|
@reyang Should I abandon this PR in favor of the separate |
I would suggest that we give 2-3 days for @CodeBlanch to advise here. He has the best knowledge about the PrometheusExporter Middleware. Also adding @alanwest and @cijothomas in case they have a strong preference. |
We briefly mentioned in last SIG meeting. PrometheusExporter is a bit lower in priority compared to overall Metrics SDK/OTLP Exporter, as we'll likely release Prometheus Stable after the stable SDK (spec is also moving in such direction that Prometheus would follow after overall SDK spec..). In short - need some more time to get to prometheus and iron out its limitations/issues, hopefully @CodeBlanch can help once get some free cycles to chime in here. |
|
@tillig Sorry for the delay in looking at this! I totally get the ask. But I think you can make this work today as-is (with Let's say I want to run my main API with HTTPS on port 5000 at the root (
|
|
@CodeBlanch So, let's say I already have some things that I integrate with that look like this: app.UseHealthCheck(5001, "/healthz");
app.UseReadinessCheck(5001, "/readiness");Those effectively boil down to things that do app.MapWhen(
ctx => ctx.Connection.LocalPort == 5001 && ctx.Request.Path == "/healthz",
b => b.UseMiddleware<HealthCheckMiddleware>());like that. I'm trying to integrate this in there, too, so what I need to be able to do (to make sure people don't hose things up by accidental ordering challenges) is something like: // Using /my-metrics as the endpoint to illustrate we want control over that AND the port.
app.MapWhen(
ctx => ctx.Connection.LocalPort == 5001 && ctx.Request.Path == "/my-metrics",
b => b.UseMiddleware<PrometheusExporterMiddleware>());The challenge is that there's no way right now to configure the location the middleware is on without doing that through // In ConfigureServices I need to set the options...
services.AddOptions<PrometheusExporterOptions>()
.Configure(opt =>
{
opt.StartHttpListener = false;
opt.ScrapeEndpointPath = "/my-metrics"
});
// ...but I also need that same value in the MapWhen
app.UseHealthCheck(5001, "/healthz");
app.UseReadinessCheck(5001, "/readiness");
app.MapWhen(
ctx => ctx.Connection.LocalPort == 5001 && ctx.Request.Path == "/my-metrics",
b => b.UseOpenTelemetryPrometheusScrapingEndpoint());Having to put it in two spots like that is a recipe for "why didn't this work?" challenges for some folks who are maybe less diligent about details or who may put a typo in there or forget about having to make them match. Separating the middleware from the endpoint configuration is important to have the control. If all This reminds me a bit of an issue we faced a while back with Autofac OWIN integration. We had an all-in-one mechanism for setting up Autofac lifetime scopes and then running the middleware that was registered in the container. It made for a really nice API that hid a lot of the dirty details from the consumer. However, we found there were some real use cases where someone wanted the Autofac lifetime scope to be set up in one spot, then run some of their own middleware, then run specific middleware that was registered in the container, then more custom stuff, then the remaining bits from the container. It became important to provide more control over the building of the pipeline to allow for those more advanced use cases. I feel like this is a similar situation. I get that there's a desire to sort of abstract away things behind a clean public API, but the flexibility to do the more advanced use cases like what I'm describing isn't there, or if it is, it brings with it challenges like "specify the config twice and make sure it all matches or things will be messed up." Perhaps the answer is another overload of public static IApplicationBuilder UseOpenTelemetryPrometheusScrapingEndpoint(this IApplicationBuilder app, string path, MeterProvider meterProvider = null)
{
if (!path.StartsWith("/"))
{
path = $"/{path}";
}
return app.Map(
new PathString(path),
builder => builder.UseMiddleware<PrometheusExporterMiddleware>(meterProvider ?? app.ApplicationServices.GetRequiredService<MeterProvider>()));
}However, this totally ignores the options that might have been configured before, which is kind of confusing, too, from an API standpoint. Which then makes me wonder if having the options specify a location is even interesting. I guess it is if you're using the standalone listener, but when I'm building my pipeline, that's when I'm setting up my endpoint routing and middleware, and that's where I need control. In the end, I was just trying to do the least-breaking-thing in the existing structure; but if there's opportunity to totally rethink/redesign it, maybe having different options for different "listener types" (the standalone listener, the middleware) would be good - split them up. Then take the path part of the options out when it comes to middleware and let folks configure it in the |
|
@tillig I hear you! The reason we don't want to make it Let me ask, why do you need to do this? app.MapWhen(
ctx => ctx.Connection.LocalPort == 5001 && ctx.Request.Path == "/my-metrics",
b => b.UseOpenTelemetryPrometheusScrapingEndpoint());If you made it... app.MapWhen(
ctx => ctx.Connection.LocalPort == 5001,
b => b.UseOpenTelemetryPrometheusScrapingEndpoint());...it would do essentially the same thing. Meaning, you don't really need to check the path in the If you really want to specify path there, yes, duplicating the value would bug me too 😄 What if you did this? var prometheusExporterOptions = app.Services.GetRequiredService<IOptions<PrometheusExporterOptions>>().Value;
app.UseHealthCheck(5001, "/healthz");
app.UseReadinessCheck(5001, "/readiness");
app.MapWhen(
ctx => ctx.Connection.LocalPort == 5001
&& ctx.Request.Path == prometheusExporterOptions.ScrapeEndpointPath,
b => b.UseOpenTelemetryPrometheusScrapingEndpoint());Doesn't look very friendly but you could move that all into an extension something like: public static IApplicationBuilder UseMetrics(this IApplicationBuilder app, int port)
{
var prometheusExporterOptions = app.Services.GetRequiredService<IOptions<PrometheusExporterOptions>>().Value;
return app.MapWhen(
ctx => ctx.Connection.LocalPort == port
&& ctx.Request.Path == prometheusExporterOptions.ScrapeEndpointPath,
b => b.UseOpenTelemetryPrometheusScrapingEndpoint());
}I do think it is reasonable for the official extension to provide the ability to do this type of thing. Let me know your thoughts on this: #3029 |
I'm not sure how you're not going to have to maintain it forever even now. I mean, I guess I could see the potential to switch it [somehow?] from middleware to a controller or something, but I also think the only reason middleware would stop working or stop being something you'd need to maintain in general is if ASP.NET Core itself changed in such a major way that middleware became not a thing. If that happened, it'd likely be as different as legacy ASP.NET + OWIN or WCF, in which case it's a whole new way to adapt to the pipeline anyway. A potentially better option is sort of what I was talking about at the end of my comment, like: First, change public class PrometheusExporterOptions
{
public int ScrapeResponseCacheDurationMilliseconds { get; set; }
}This removes the endpoint path from options so it can be used in routing or endpoint/pipeline setup; and it lets you also separately configure the standalone HTTP listener if you want to use that. That would also serve to remove the need for Change // Control to use MapWhen or do what you want if you need it...
app.Map("/metrics", b => b.UseOpenTelemetryPrometheusScrapingEndpoint());
// ...or use the one-liner.
app.UseOpenTelemetryPrometheusScrapingEndpoint("/metrics");Maybe it's two different named methods? Unclear. Point being, the options don't have the endpoint info in them anymore and you can control it a bit more during pipeline definition. If you still want to use a standalone HTTP listener, maybe instead of having that "baked into" the using var meterProvider = Sdk.CreateMeterProviderBuilder()
.AddMeter("MyCompany.MyProduct.MyLibrary")
.AddPrometheusExporterWithListener("http://localhost:1234/metrics")
.Build();I mean, it could be an overload, too, where the listener is implicitly started if you pass that URL. using var meterProvider = Sdk.CreateMeterProviderBuilder()
.AddMeter("MyCompany.MyProduct.MyLibrary")
.AddPrometheusExporter(endpoint: "http://localhost:1234/metrics")
.Build();Again, point there is it's explicit and not tied to the exporter itself. Anyway, that would split out:
... and it'd give the control needed for the pipeline without exposing the middleware directly. |
|
@tillig I just replied on the other PR about some of the use cases I think we should build for and why having As far as what to do with |
|
With #3029 coming in, I don't think we'll need this anymore. There should be enough control to do what needs to be done with the new extensions; or, if not, we can build on that. |
4 participants
Fixes #2776.
Changes
Updated
PrometheusExporterMiddlewareto be public rather than internal.Notes
While the current
UseOpenTelemetryPrometheusScrapingEndpointallows configuration of the Prometheus middleware based on options, it doesn't allow for more flexible options or more tailored handling of the Prometheus endpoint.My specific use case is that I need to do
app.MapWhenand include not only path but also port. I can't use the standalone listener on that port because I have other management things like health checks also exposed on that port; but I can't put the/metricsendpoint right on the main API listener port, either.Other options I considered before going with making
PrometheusExporterMiddlewarepublic:PrometheusExporterOptions: This seemed like it'd be confusing given there were alreadyHttpListenerPrefixesin there.UseOpenTelemetryPrometheusScrapingEndpointoverload that takes a predicate: Something where you can pass theFunc<HttpContext, bool>that you pass toapp.MapWhen. However, that would end up bypassing thePrometheusExporterOptionsentirely because the predicate would control the whole mapping. It'd be up to the consumer to resolve the options and build the predicate. This didn't seem intuitive.In the end it seemed like the one thing that would solve all the problems for advanced users while still maintaining the nice, easy
UseOpenTelemetryPrometheusScrapingEndpointmechanism was to makePrometheusExporterMiddlewarepublic. Advanced users will know that they'll need to manage theUseMiddleware<T>and parameter setup; folks looking for the one-liner will still have that ability.Since the middleware got switched to public I also moved it into the main folder of the OpenTelemetry.Exporter.Prometheus project since it appeared everything in the "Implementation" folder was internal.