[Exporter.PrometherusHttpListener] Restore ScrapeResponseCacheDuratio…

[Kielek]

Fixes also #3679

Changes

Restore ScrapeResponseCacheDurationMilliseconds

For significant contributions please make sure you have completed the following items:

• Appropriate CHANGELOG.md updated for non-trivial changes
• [ ] Design discussion issue #
• [ ] Changes in public API reviewed

[reyang]

Why do we need to restore it?

[codecov]

Codecov Report

Merging #3694 (b57ca7f) into main (d10f1f9) will decrease coverage by 0.06%.
The diff coverage is 88.88%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3694      +/-   ##
==========================================
- Coverage   87.80%   87.74%   -0.07%     
==========================================
  Files         283      284       +1     
  Lines       10308    10314       +6     
==========================================
- Hits         9051     9050       -1     
- Misses       1257     1264       +7     

Impacted Files	Coverage Δ
...HttpListener/Internal/PrometheusExporterOptions.cs	100.00% <ø> (ø)
...us.HttpListener/Internal/PrometheusHttpListener.cs	80.00% <ø> (ø)
...ometheus.AspNetCore/PrometheusAspNetCoreOptions.cs	75.00% <75.00%> (ø)
...heus.HttpListener/PrometheusHttpListenerOptions.cs	90.90% <75.00%> (-9.10%)	⬇️
.../PrometheusExporterApplicationBuilderExtensions.cs	100.00% <100.00%> (ø)
...rometheusExporterEndpointRouteBuilderExtensions.cs	100.00% <100.00%> (ø)
...rometheusExporterMeterProviderBuilderExtensions.cs	100.00% <100.00%> (ø)
...etheus.HttpListener/Internal/PrometheusExporter.cs	100.00% <100.00%> (ø)
...theusHttpListenerMeterProviderBuilderExtensions.cs	92.85% <100.00%> (ø)
src/OpenTelemetry/Logs/Pool/LogRecordSharedPool.cs	78.94% <0.00%> (-21.06%)	⬇️
... and 9 more

[Kielek]

Why do we need to restore it?

@reyang, this option was available in the previous version (when both HttpListener and AspNetCore were in one package).

I think that it is useful, to have a possibility to cache data to avoid to frequent calculation of needed data.

[reyang]

I think that it is useful, to have a possibility to cache data to avoid to frequent calculation of needed data.

I know it is useful, do we even want to encourage feature parity between HttpListener and ASP.NET Core binding? I would say no, I think we should push hard for ASP.NET Core binding for any production scenario, and limit HttpListener to only dev inner loop and testing, in a nutshell, I don't think feature parity is something we should be driving here.

[Kielek]

I know it is useful, do we even want to encourage feature parity between HttpListener and ASP.NET Core binding? I would say no, I think we should push hard for ASP.NET Core binding for any production scenario, and limit HttpListener to only dev inner loop and testing, in a nutshell, I don't think feature parity is something we should be driving here.

@reyang, I understand your perspective, but in Auto Instrumentation we would like to have fully functional, production ready Prometherus exporter without referencing unnecessary, additional libraries. It is not always possible with AspNetCore based. Especially it is not possible for any .NET Framework which is still significant part of the usages.

[pellared]

I think we should push hard for ASP.NET Core binding for any production scenario, and limit HttpListener to only dev inner loop and testing

We should not limit ourselves to ASP.NET Core. There are many other types of .NET services that are NOT HTTP servers. Examples:

• queue "workers" consuming messages from message-brokers, also actors (in "actor model")
• Windows services
• WCF servers
• servers using any non-HTTP protocol

[CodeBlanch]

LGTM

[reyang]

I know it is useful, do we even want to encourage feature parity between HttpListener and ASP.NET Core binding? I would say no, I think we should push hard for ASP.NET Core binding for any production scenario, and limit HttpListener to only dev inner loop and testing, in a nutshell, I don't think feature parity is something we should be driving here.

@reyang, I understand your perspective, but in Auto Instrumentation we would like to have fully functional, production ready Prometherus exporter without referencing unnecessary, additional libraries. It is not always possible with AspNetCore based. Especially it is not possible for any .NET Framework which is still significant part of the usages.

Good point, if the goal is to make the HttpListener production ready, we should have a design for authentication. If there is no authentication, it already means that the exporter is only going to serve a trusted environment, which makes caching less useful.

[reyang]

I think we should push hard for ASP.NET Core binding for any production scenario, and limit HttpListener to only dev inner loop and testing

We should not limit ourselves to ASP.NET Core. There are many other types of .NET services that are NOT HTTP servers. Examples:

• queue "workers" consuming messages from message-brokers, also actors (in "actor model")
• Windows services
• WCF servers
• servers using any non-HTTP protocol

You've got me convinced 😄

[reyang]

Need to understand the overall design/plan #3694 (comment)

[Kielek]

@reyang, I think that we should keep authentication out of scope of this PR.
In Auto Instrumentation we are trying to keep OTel overhead as minimal as possible. In such condition, the caching mechanism is still useful, even in trusted environments.

If you are fine with this, I can create issue/feature request to cover authorization and authentication process for Prometheus over HttpListener.
Based on some research, there is a possibility to set up authentication in HttpListener: https://learn.microsoft.com/en-us/dotnet/api/system.net.authenticationschemes?view=net-7.0

In addition, we should give a possibility to bind over https and set up correct certificate.

[reyang]

@reyang, I think that we should keep authentication out of scope of this PR. In Auto Instrumentation we are trying to keep OTel overhead as minimal as possible. In such condition, the caching mechanism is still useful, even in trusted environments.

Do you have actual measurements which show why the caching behavior is important for auto-instrumentation? I wish to understand why it is important and how important it is.

[reyang]

@reyang, I think that we should keep authentication out of scope of this PR.

I agree that we should keep this PR scoped. However, I think we should not merge this PR until there is an authentication support.

[pellared]

Do you have actual measurements which show why the caching behavior is important for auto-instrumentation? I wish to understand why it is important and how important it is.

To mitigate "potential DoS". We want to have a possibility to "control" the performance. I think it is of the same importance as for the OpenTelemetry.Exporter.Prometheus.AspNetCore. (sorry for my English)

I think we should not merge this PR until there is an authentication support

Why so? How will authentication help without authorization?

Personally, I feel that HTTPS support is more important to encrypt the traffic. Maybe self-signed HTTPS could be used by default. Still, I think that none of that is coupled with caching.

[pellared]

@reyang ☝️ (I have forgotten to add a mention 😆 )

[reyang]

Do you have actual measurements which show why the caching behavior is important for auto-instrumentation? I wish to understand why it is important and how important it is.

To mitigate "potential DoS". We want to have a possibility to "control" the performance. I think it is of the same importance as for the OpenTelemetry.Exporter.Prometheus.AspNetCore. (sorry for my English)

"Potential DoS" is already a production topic, which requires a combination of security/perf(caching), I don't think "just having a caching" would solve DoS - instead, it is giving people false assumption that it can be used in production while actually that's not the case. The analogy is installing airbags to a toy cart, if the intention is to make it secure, we should do proper design.

[pellared]

I do not claim that it is production-ready, I just thought it is not "against" it 😉 But I understand that it makes more sense for you to first have a design in place to drive the implementation.

[reyang]

Personally, I feel that HTTPS support is more important to encrypt the traffic. Maybe self-signed HTTPS could be used by default. Still, I think that none of that is coupled with caching.

I agree that HTTPS is important, I actually think that authentication/authorization makes less sense on plain HTTP (and that's why most websites moved to HTTPS). I think all these should be addressed in the HttpListener version of PrometheusExporter, before we can give signal that it is production ready. And this requires some design and overall thinking, which is exactly I'm asking #3694 (review). Without an overall design/thinking/plan, I don't feel we should merge this PR.

[Kielek]

Closing,
Please look into #3728 if it is worth to invest more time on it.