Add Prometheus text format serializers. #4178

anuraaga · 2022-02-16T08:56:16Z

With metrics nearing stability, I think coupling the exporter to the prometheus client library has some risk as it may not make it in time. It's nice to also get some performance by directly serializing given how huge they can be (in particular, lots of arrays allocated for histograms in the adapter code that are eliminated). Many Java apps have a strong correlation between full GC and Prometheus scrapes :) I will move the Collector integration into an alpha extension instead so it can be decoupled from a stable release.

This only adds serialization logic, it doesn't use it yet or remove a few smaller usages of the client library that are left which will come in followups.

Maintenance-wise, given the above benefits, I think 414 LoC of the serializer vs 340 LoC for the MetricAdapter is not that much more. Code is not quite as simple but prometheus format is relatively straight forward so doesn't seem too bad.

The output makes sure to match the client library 100% for openmetrics. It has some small deviations for legacy prometheus format which are compatible with prometheus, presumably the client library didn't mess with that format to avoid breaking unit tests relying on the strings.

codecov · 2022-02-16T09:17:25Z

Codecov Report

Merging #4178 (e26d09e) into main (6266b16) will decrease coverage by 0.04%.
The diff coverage is 86.81%.

@@             Coverage Diff              @@
##               main    #4178      +/-   ##
============================================
- Coverage     90.28%   90.24%   -0.05%     
- Complexity     4611     4653      +42     
============================================
  Files           537      539       +2     
  Lines         14097    14326     +229     
  Branches       1348     1370      +22     
============================================
+ Hits          12728    12929     +201     
- Misses          926      943      +17     
- Partials        443      454      +11

Impacted Files	Coverage Δ
.../opentelemetry/exporter/prometheus/Serializer.java	`86.15% <86.15%> (ø)`
...ntelemetry/exporter/prometheus/PrometheusType.java	`91.66% <91.66%> (ø)`
...entelemetry/exporter/prometheus/MetricAdapter.java	`92.42% <100.00%> (ø)`
...ension/trace/jaeger/sampler/OkHttpGrpcService.java	`75.30% <0.00%> (-6.18%)`	⬇️
...metry/sdk/autoconfigure/ResourceConfiguration.java	`92.59% <0.00%> (+4.35%)`	⬆️
...exporter/jaeger/MarshalerCollectorServiceGrpc.java	`90.47% <0.00%> (+4.76%)`	⬆️
...metry/exporter/prometheus/PrometheusCollector.java	`100.00% <0.00%> (+13.04%)`	⬆️
...entelemetry/exporter/jaeger/PostSpansResponse.java	`100.00% <0.00%> (+100.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6266b16...e26d09e. Read the comment docs.

jsuereth · 2022-02-16T17:02:34Z

Maintenance-wise, given the above benefits, I think 414 LoC of the serializer vs 340 LoC for the MetricAdapter is not that much more. Code is not quite as simple but prometheus format is relatively straight forward so doesn't seem too bad.

My main concern is not about serializing in prometheus format, but whether or not we keep up-to-date with Prometheus Optimisations (e.g. filtering which metrics are returned via HTTP request headers, etc.). Given we already opt-in to doing that work, via maintaining our own HTTP server, I think it's only logical to do the serialization as well. Agree it should be minimal code and hopefully reduce memory usage.

jsuereth · 2022-02-16T17:06:38Z

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Serializer.java

+    void writeExemplar(
+        Writer writer, Collection<ExemplarData> exemplars, double minExemplar, double maxExemplar)
+        throws IOException {
+      for (ExemplarData exemplar : exemplars) {


I believe this code should also be writing dropped attributes as exemplar labels (after your trace_id/span_id code, with a limit on total size of labels written to prometheus).

Don't think we do that right now

https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/MetricAdapter.java#L327

Would like to keep this pr to just reproducing our current behavior and we can add features in the future

Yes, this is because I was waiting for the Attribute -> Label spec to finalize in prometheus. Fine to push to future PR, but I do wish I had added that in the original hook.

jsuereth · 2022-02-17T14:35:47Z

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusType.java

+      case EXPONENTIAL_HISTOGRAM:
+        return HISTOGRAM;
+    }
+    throw new IllegalArgumentException(


Will this cause the entire prometheus export to crash, or is this handled in the exporter later?

Its not wired up to an exporter yet so I believe that's a decision that is yet to be made.

This can never be thrown when the BOM is used, if it did happen to be thrown then the export would fail. I've seen various linkage exceptions related to unaligned dependencies and think it's mostly not possible to use OTel without alignment so leant towards just making this an exception instead of log / fallback, but either approach would be fine here.

jsuereth · 2022-02-17T14:36:16Z

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Serializer.java

+  }
+
+  private void write(MetricData metric, Writer writer) throws IOException {
+    // TODO: Implement


Can you open a bug for this one?

I suspect this would be an issue in the spec repo. I changed this comment a bit to reflect this proposed spec wording

https://github.com/open-telemetry/opentelemetry-specification/pull/2266/files#diff-0efae13f08f98e62a81767d5daeff37ebb7ef8c50537c7b9013e72506e9b055aR1152

jsuereth · 2022-02-17T14:42:42Z

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Serializer.java

+          valueAtPercentile.getValue(),
+          point.getAttributes(),
+          point.getEpochNanos(),
+          "quantile",


This a note for me, doubting my own code here.

Percentile != Quantile. We had this issue in OpenCensus where we needed to switch from [0.0,1.0] to [0.0, 100.0].

However, OTLP specifies summaries in Quantile. Is this an instance where Java SDK is uses the wrong name? I never noticed before, as I didn't pay close attention until working on some OpenCensus-Go bridges.

Wikipedia tells me that a percentile is a particular type of quantile, so is this a problem?

I think @jsuereth is referring to the fact that the OTel quantile is not actually supposed to be that particular type of quantile. Spec says

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#summary-legacy

within the interval [0.0, 1.0]

We would at least need to change the name of the class to ValueAtQuantile and this obviously wrong javadoc

https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/ValueAtPercentile.java#L21

Would also need to make sure we don't have any logic such as validation that is using the percentile expectation

jsuereth · 2022-02-17T14:43:27Z

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Serializer.java

+            public void accept(AttributeKey<?> key, Object value) {
+              try {
+                if (wroteOne) {
+                  writer.write(',');


Would be nice to get test coverage for multi-attributes.

jack-berg · 2022-02-17T22:48:53Z

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusType.java

+      case EXPONENTIAL_HISTOGRAM:
+        return HISTOGRAM;
+    }
+    throw new IllegalArgumentException(


Its not wired up to an exporter yet so I believe that's a decision that is yet to be made.

jack-berg · 2022-02-17T22:51:57Z

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusType.java

+            && longSumData.getAggregationTemporality() == AggregationTemporality.CUMULATIVE) {
+          return COUNTER;
+        }
+        return GAUGE;


Is it possible for a prometheus collector which prefers cumulative temporality to get non-monotonic, delta, sum data?

I believe after we added preferred aggregation support it isn't possible when using our SDK. It could be possible if someone used the exporter without our SDK though (creating arbitrary MetricData).

jack-berg · 2022-02-17T23:04:58Z

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/Serializer.java

+          valueAtPercentile.getValue(),
+          point.getAttributes(),
+          point.getEpochNanos(),
+          "quantile",


Wikipedia tells me that a percentile is a particular type of quantile, so is this a problem?

anuraaga requested review from carlosalberto, jack-berg, jkwatson and jsuereth as code owners February 16, 2022 08:56

anuraaga requested a review from a user February 16, 2022 08:56

anuraaga requested a review from Oberon00 as a code owner February 16, 2022 08:56

anuraaga force-pushed the prometheus-004-serializer branch 2 times, most recently from d3ca6e1 to 7c68315 Compare February 16, 2022 08:57

Add Prometheus text format serializers.

1f49ebb

anuraaga force-pushed the prometheus-004-serializer branch from 7c68315 to 1f49ebb Compare February 16, 2022 09:14

jsuereth reviewed Feb 16, 2022

View reviewed changes

jsuereth approved these changes Feb 17, 2022

View reviewed changes

jack-berg approved these changes Feb 17, 2022

View reviewed changes

Attributes coverage

e26d09e

anuraaga merged commit 1f2379f into open-telemetry:main Feb 18, 2022

anuraaga mentioned this pull request Feb 22, 2022

Change ValueAtPercentile to ValueAtQuantile #4206

Closed

jack-berg mentioned this pull request Nov 27, 2023

Exponential Histogram support to the Prometheus exporter #6015

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Prometheus text format serializers. #4178

Add Prometheus text format serializers. #4178

anuraaga commented Feb 16, 2022 •

edited

Loading

codecov bot commented Feb 16, 2022 •

edited

Loading

jsuereth commented Feb 16, 2022

jsuereth Feb 16, 2022

anuraaga Feb 17, 2022

jsuereth Feb 17, 2022

jsuereth Feb 17, 2022

jack-berg Feb 17, 2022

anuraaga Feb 18, 2022

jsuereth Feb 17, 2022

anuraaga Feb 18, 2022

jsuereth Feb 17, 2022

jack-berg Feb 17, 2022

anuraaga Feb 18, 2022

jsuereth Feb 17, 2022

jack-berg Feb 17, 2022

jack-berg Feb 17, 2022

anuraaga Feb 18, 2022

jack-berg Feb 17, 2022

Add Prometheus text format serializers. #4178

Add Prometheus text format serializers. #4178

Conversation

anuraaga commented Feb 16, 2022 • edited Loading

codecov bot commented Feb 16, 2022 • edited Loading

Codecov Report

jsuereth commented Feb 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Feb 16, 2022 •

edited

Loading

codecov bot commented Feb 16, 2022 •

edited

Loading