Output EOF following the OpenMetrics exposition recommendation

[reyang]

Changes

• Added support for EOF.

Reference: https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#overall-structure

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

• Appropriate CHANGELOG.md updated for non-trivial changes

[alanwest]

I don't know enough about prometheus, but I assume this won't cause an older prometheus scrapper to blow up?

Also, is simply adding # EOF enough to warrant a change to the content-type to application/openmetrics-text?

opentelemetry-dotnet/src/OpenTelemetry.Exporter.Prometheus.AspNetCore/PrometheusExporterMiddleware.cs

Line 76 in a87f8f7

response.ContentType = "text/plain; charset=utf-8; version=0.0.4";

[reyang]

I don't know enough about prometheus, but I assume this won't cause an older prometheus scrapper to blow up?

I've tested this with older version of scrappers.

Also, is simply adding # EOF enough to warrant a change to the content-type to application/openmetrics-text?

opentelemetry-dotnet/src/OpenTelemetry.Exporter.Prometheus.AspNetCore/PrometheusExporterMiddleware.cs

Line 76 in a87f8f7

response.ContentType = "text/plain; charset=utf-8; version=0.0.4";

I guess the answer is no, I keep learning new things while reading the OpenMetrics spec, e.g. #3651. I think we need to do a full scan of the spec, maybe it's okay to flip the content-type earlier?

[alanwest]

maybe it's okay to flip the content-type earlier?

Probably fine to wait since content-type might be a thing that would affect older scrapers. But may become necessary with exemplar support?

[codecov]

Codecov Report

Merging #3654 (82187e5) into main (3a8f139) will decrease coverage by 0.31%.
The diff coverage is 33.33%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3654      +/-   ##
==========================================
- Coverage   87.88%   87.57%   -0.32%     
==========================================
  Files         283      283              
  Lines       10264    10276      +12     
==========================================
- Hits         9021     8999      -22     
- Misses       1243     1277      +34     

Impacted Files	Coverage Δ
...tpListener/Internal/PrometheusCollectionManager.cs	74.72% <20.00%> (-5.77%)	⬇️
...heus.HttpListener/Internal/PrometheusSerializer.cs	80.83% <100.00%> (+0.49%)	⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs	35.71% <0.00%> (-42.86%)	⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	36.36% <0.00%> (-40.91%)	⬇️
...Listener/Internal/PrometheusExporterEventSource.cs	16.66% <0.00%> (-11.12%)	⬇️
...metheus.AspNetCore/PrometheusExporterMiddleware.cs	60.71% <0.00%> (-7.15%)	⬇️
...metryProtocol/Implementation/ActivityExtensions.cs	91.75% <0.00%> (-3.30%)	⬇️
....Prometheus.HttpListener/PrometheusHttpListener.cs	80.00% <0.00%> (-2.67%)	⬇️
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs	86.66% <0.00%> (-2.23%)	⬇️
... and 1 more

[Kielek]

I do not check the specification, but it is worth to double check how it should behave when there is no metrics to export.
Should it be 200 with empty response or 200 with # EOF\n?

Ref: #3643

[reyang]

I do not check the specification, but it is worth to double check how it should behave when there is no metrics to export. Should it be 200 with empty response or 200 with # EOF\n?

Ref: #3643

Good point, the current approach is returning HTTP 200 without content/content-type, so technically it is not a Prometheus nor OpenMetrics exposition format (which means OpenMetrics spec won't apply).