add Exemplar support for OpenTelemetry tracing #652

fstab · 2021-04-12T21:43:36Z

New Section in README:

Exemplars are a feature of the OpenMetrics format that allows applications to link metrics
to example traces. Starting with version 0.11.0, client_java automatically adds Exemplars for applications that
are instrumented with OpenTelemetry distributed tracing. No code change is required.

Running an Exemplars Example

If you want to see this in action, you can run the example from the ExemplarsClientJavaIT:

mvn package
cd integration_tests/exemplars_otel_agent/target/
curl -LO https://github.com/open-telemetry/opentelemetry-java-instrumentation/releases/download/v1.0.1/opentelemetry-javaagent-all.jar
java -Dotel.traces.exporter=logging -Dotel.metrics.exporter=none -javaagent:./opentelemetry-javaagent-all.jar -jar ./sample-rest-application.jar

Now you have a Spring REST service running on http://localhost:8080/hello that is instrumented with the OpenTelemetry Java agent.

In order to get metrics in OpenMetrics format, run

curl -H 'Accept: application/openmetrics-text; version=1.0.0; charset=utf-8' http://localhost:8080/metrics

You should see metrics with Exemplars, for example in the request_duration_histogram metric:

request_duration_histogram_bucket{path="/god-of-fire",le="0.004"} 4.0 # {trace_id="043cd631811e373e4180a678c06b128e",span_id="cd122e457d2ca5b0"} 0.0033 1618261159.027

Note that this is an example application for a unit test, so durations don't represent real durations, and some example metrics might not make sense in the real world.

Enabling And Disabling Exemplars

By default, Exemplars are enabled if OpenTelemetry tracing is detected. You can disable this globally with:

ExemplarConfig.disableByDefault();

If you want to enable Exemplars only for a single metric, use withExemplarSampler() in the metric builder.

// This also works with Gauges, Histograms, and Summaries.
Counter labelsCustomExemplar = Counter.build()
    .name("number_of_events_total")
    .help("help")
    .withExemplarSampler(new MyExemplarSampler())
    ...
    .register();

Likewise, you can disable Exemplars for a single metric like this:

// This also works with Gauges, Histograms, and Summaries.
Counter labelsCustomExemplar = Counter.build()
    .name("number_of_events_total")
    .help("help")
    .withoutExemplars()
    ...
    .register();

See ExemplarTest for more examples.

Implement Your Own Exemplar Sampling Algorithm

The DefaultExemplarSampler is very simple: It samples a new Exemplar if the current one is older than 7 seconds.
You might want to implement your own Exemplar sampler that provides more interesting Exemplars for your application.
In order to do so, just implement the following interfaces:

CounterExemplarSampler
GaugeExemplarSampler
HistogramExemplarSampler
SummaryExemplarSampler

You can set your implementations as default like this:

ExemplarConfig.setDefaultCounterExemplarSampler(mySampler);
ExemplarConfig.setDefaultGaugeExemplarSampler(mySampler);
ExemplarConfig.setDefaultHistogramExemplarSampler(mySampler);
ExemplarConfig.setDefaultSummaryExemplarSampler(mySampler);

Implement Support for Other Tracing Vendors

Version 0.11.0 implements support for OpenTelemetry. If you are a vendor for another distributed tracer,
please create a pull request adding support for your system. The idea is that copy-and-paste the
tracer_otel module and modify it for your needs. If this turns out to be too simplistic, please create
a GitHub issue and let us know what you need.

Implementation Notes

This PR also introduces integration tests with testcontainers, which is a framework for running tests against Docker containers. Benefits:

This allows tests like the ExemplarsClientJavaIT where we attach the OpenTelemetry Java agent to the system under test.
This allows tests with different Java versions, like JavaVersionsIT that can be used to ensure the library is compatible with Java 6 or IBM's OpenJ9.

That means docker must be installed on the system where you run mvn verify.

brian-brazil

There's a few things here that don't jive with the OM spec.

brian-brazil · 2021-04-12T22:37:38Z

README.md

+In order to do so, just implement the following interfaces:
+
+* `CounterExemplarSampler`
+* `GaugeExemplarSampler`


Gauges do not support exemplars.

brian-brazil · 2021-04-12T22:37:48Z

README.md

+* `CounterExemplarSampler`
+* `GaugeExemplarSampler`
+* `HistogramExemplarSampler`
+* `SummaryExemplarSampler`


Summarys do not support exemplars.

brian-brazil · 2021-04-12T22:38:55Z

exemplars_parent/exemplars/src/main/java/io/prometheus/client/exemplars/api/Exemplar.java

+  private final double value;
+  private final long timestampMs;
+
+  public Exemplar(String traceId, String spanId, double value, long timestampMs) {


This is not the data model of an examplar in OpenMetrics, they can have arbitrary labelsets as long as they're within the character limit. Timestamps are also optional.

brian-brazil · 2021-04-12T22:41:43Z

exemplars_parent/exemplars/src/main/java/io/prometheus/client/exemplars/api/Value.java

+/**
+ * Lazy value holder for a double value.
+ * <p/>
+ * Counters and Gauges track their current value using a DoubleAdder, so getting the value


Why would you need the value of a metric when sampling exemplars and thus care that the implementation uses a DoubleAdder? Only the size of the observation itself should matter.

I changed the CounterExemplarSampler to this

Exemplar sample(double increment, Value newTotalValue, Exemplar previous);

The default implementation uses the increment and ignores the newTotalValue. However, I still kept the newTotalValue available in the API so that users can access it if needed (e.g. start sampling after the total value exceeded a threshold).

brian-brazil · 2021-04-12T22:44:41Z

...rent/exemplars/src/main/java/io/prometheus/client/exemplars/impl/DefaultExemplarSampler.java

+  static class SystemClock implements Clock {
+    @Override
+    public long currentTimeMillis() {
+      return System.currentTimeMillis();


What's the latency impact of calling this on every observation? This tends to be a relatively slow call.

I changed the implementation so that this is only called if OpenTelemetry tracing is available.

For the latency, I added a ExemplarsBenchmark. Result on my local Linux laptop:

Benchmark Mode Samples Score Error Units i.p.b.ExemplarsBenchmark.testCounter avgt 200 28.252 ± 0.312 ns/op i.p.b.ExemplarsBenchmark.testCounterWithExemplars avgt 200 46.042 ± 0.781 ns/op i.p.b.ExemplarsBenchmark.testCounterWithoutExemplars avgt 200 29.322 ± 0.475 ns/op

so about 17ns for the currentTimeMillis() call.

brian-brazil · 2021-04-12T22:51:07Z

simpleclient_common/src/main/java/io/prometheus/client/exporter/common/TextFormat.java

-
+          writeTimestamp(writer, sample.timestampMs);
+        }
+        if (sample.exemplar != null) {


This code does not cover all potential valid exemplars.

fixed with the new Exemplar data model

Still not quite right, 0 is a valid timestamp.

Ok, using Long now and null to indicate no timestamp.

brian-brazil · 2021-04-12T22:54:09Z

exemplars_parent/exemplars/src/main/java/io/prometheus/client/exemplars/api/Exemplar.java

+/**
+ * Immutable data class holding an Exemplar.
+ */
+public class Exemplar {


I would suggest having the definition of Exemplar itself live within simpleclient, as that's where all of the rest of the data model is.

The ExemplarSampler implementations use the Exemplar data model, so if we move the model to simpleclient, we will need to move all ExemplarSampler implementations to simpleclient as well. Currently it's not much, but if in the future people provide more sophisticated implementations it might be worthwhile to have this in a separate module.

My point is that the core data model should all live in one place, not spread across N modules that have to be pieced together by someone trying to work with this. It's kinda like having an entire module just to handle timestamps.

Personally I wouldn't have the core simpleclient depending on any other modules at all, rather make this something that the user has to add an extra module to enable, rather than silently creating the possibility for dependency hell on upgrade. simpleclient thus far has purposefully avoided any dependencies for that reason, it should always be safe to have simpleclient instrumentation in place.

This is also still not the data model of an OM exemplar, as timestamps are optional.

brian-brazil · 2021-04-12T23:01:46Z

simpleclient_common/src/main/java/io/prometheus/client/exporter/common/TextFormat.java

        }
        writer.write('\n');
      }
    }
    writer.write("# EOF\n");
  }

+  private static void writeTimestamp(Writer writer, long timestampMs) throws IOException {


omWriteTimestamp would avoid confusion, as this is OM specific logic.

fstab · 2021-04-18T23:11:35Z

Thanks a lot @brian-brazil for reviewing this, I appreciate it a lot. I pushed an update and I'd be happy if you could review it once more.

brian-brazil

All public methods should have javadoc.

brian-brazil · 2021-04-19T08:21:21Z

benchmark/pom.xml

@@ -49,7 +49,7 @@
        <dependency>
            <groupId>io.prometheus</groupId>
            <artifactId>simpleclient</artifactId>
-            <version>0.10.1-SNAPSHOT</version>
+            <version>${project.version}</version>


This will break some build systems, I forget which one but that's the reason all versions are explicit.

brian-brazil · 2021-04-19T08:22:00Z

benchmark/src/main/java/io/prometheus/benchmark/ExemplarsBenchmark.java

@@ -0,0 +1,80 @@
+package io.prometheus.benchmark;


The readme should be updated with the new numbers.

I'd re-run all the benchmarks, so it's apples to apples.

This comment still stands.

brian-brazil · 2021-04-19T08:22:40Z

...arent/exemplars/src/main/java/io/prometheus/client/exemplars/api/CounterExemplarSampler.java

+
+  /**
+   * @param increment     the value added to the counter on this event
+   * @param newTotalValue the new total counter value after adding the increment.


Why would you care about this? Increments should be independent.

I'm not sure about this. Here's an example that might be a valid use case: A counter representing a buffer size. If the buffer can only grow, a counter should be a valid metric. A user might decide to sample exemplars only when the buffer has exceeded a certain size, thus the exemplar sampler needs the total value.

I'm not saying that this will be common, in most cases users won't need the total value. However, if we remove it from the API there will be no chance to use it even if there are rare cases where it makes sense.

Anyway, I don't have a strong opinion on this. If you still think removing it would be better I'm happy to remove it.

The interesting thing about a counter is how quickly it goes up, the absolute value doesn't matter.

A counter representing a buffer size

That's a gauge, as you care about the absolute value.

brian-brazil · 2021-04-19T08:23:27Z

exemplars_parent/exemplars/src/main/java/io/prometheus/client/exemplars/api/Exemplar.java

+/**
+ * Immutable data class holding an Exemplar.
+ */
+public class Exemplar {


My point is that the core data model should all live in one place, not spread across N modules that have to be pieced together by someone trying to work with this. It's kinda like having an entire module just to handle timestamps.

Personally I wouldn't have the core simpleclient depending on any other modules at all, rather make this something that the user has to add an extra module to enable, rather than silently creating the possibility for dependency hell on upgrade. simpleclient thus far has purposefully avoided any dependencies for that reason, it should always be safe to have simpleclient instrumentation in place.

This is also still not the data model of an OM exemplar, as timestamps are optional.

brian-brazil · 2021-04-19T08:24:35Z

exemplars_parent/exemplars/src/main/java/io/prometheus/client/exemplars/api/Exemplar.java

+   * @param labels name/value pairs. Expecting an even number of strings. The combined length of the label names and
+   *               values must not exceed 128 UTF-8 characters. Neither a label name nor a label value may be null.
+   */
+  public Exemplar(double value, String... labels) {


Passing in labels should be consistent with how it's done in Info, so a map should also be supported.

I moved the Exemplar and all classes that depend on it to simpleclient. Now only the bridges to the tracing libraries live in separate modules. I agree that it should be safe to have simpleclient instrumentation in place, and that dependencies should be avoided. I hope this is the case with the current implementation: simpleclient does not have any runtime dependencies. OpenTelemetry is available we will load the corresponding SpanContextSupplier, if not we won't.

We could disable exemplars by default and have the user explicitly enable them with a ExemplarConfig.enableExemplars() call. Apart from that I don't see how to improve this. We could load SpanContextSuppliers dynamically through a class loader, but that would yield to the same optional dependency.

I added an Exemplar constructor with a Map parameter to be consistent with Info.

brian-brazil · 2021-04-19T08:26:03Z

exemplars_parent/exemplars/src/main/java/io/prometheus/client/exemplars/api/ExemplarConfig.java

+    instance.histogramExemplarSampler = instance.noopExemplarSampler;
+  }
+
+  // Methods for internal use


If they're internal, why are they public?

I removed the comment. It makes sense to have these as public methods, because a user might want to keep a copy of the original default ExemplarSampler.

brian-brazil · 2021-04-19T08:28:39Z

exemplars_parent/exemplars/src/main/java/io/prometheus/client/exemplars/api/Exemplar.java

+    return timestampMs;
+  }
+
+  private void validateLabels(String... labels) {


You should also check that the label names are valid.

Added a check to make sure label names are unique.

brian-brazil · 2021-04-19T08:32:07Z

simpleclient_common/src/main/java/io/prometheus/client/exporter/common/TextFormat.java

-
+          writeTimestamp(writer, sample.timestampMs);
+        }
+        if (sample.exemplar != null) {


Still not quite right, 0 is a valid timestamp.

fstab · 2021-04-20T22:13:06Z

I pushed another update to make sure that simpleclient works even if none of the dependencies (tracer_common, tracer_otel, tracer_otel_agent) is present.

brian-brazil

It may make sense for an exemplar to be provided directly at inc/observe time, as exemplars are not limited to spans/traces.

brian-brazil · 2021-04-21T07:39:30Z

simpleclient/src/main/java/io/prometheus/client/exemplars/Exemplar.java

+      if (labels[i] == null) {
+        throw new IllegalArgumentException("labels[" + i + "] is null");
+      }
+      if (i % 2 == 0) { // label names should be unique


This doesn't test that the label names are valid.

Two questions:

What else would you test apart from not null and uniqueness?

Do you see any drawbacks if we sort the exemplar labels by name? That would give them a defined order when the labels are passed as a HashMap.

That the characters match the labelname regex.

Sorting is fine, but a HashMap doesn't have ordering. You'd want a TreeMap for that.

added sorting and the check for the label name regex.

brian-brazil · 2021-04-21T07:41:21Z

benchmark/src/main/java/io/prometheus/benchmark/ExemplarsBenchmark.java

@@ -0,0 +1,80 @@
+package io.prometheus.benchmark;


I'd re-run all the benchmarks, so it's apples to apples.

brian-brazil · 2021-04-21T07:42:06Z

integration_tests/exemplars_otel/pom.xml

+    <dependency>
+      <groupId>io.opentelemetry</groupId>
+      <artifactId>opentelemetry-api</artifactId>
+      <version>${otel.version}</version>


My comment applies to all versions, avoid variables. Of course it's possible the feature has been added in the meantime, as that request was some years back.

I added the maven wrapper script so that users have a defined version of maven (specified in .mvn/wrapper/maven-wrapper.properties) if they call ./mvnw instead of mvn.

My comment stands, you can't assume that users are using maven.

brian-brazil · 2021-04-21T07:43:03Z

integration_tests/example_application/src/main/java/io/prometheus/smoketest/Server.java

@@ -0,0 +1,18 @@
+package io.prometheus.smoketest;


Shouldn't this be under client?

brian-brazil · 2021-04-21T07:43:48Z

pom.xml

@@ -43,6 +43,7 @@
    </developers>

    <modules>
+        <module>tracer_parent</module>


This doesn't follow the naming scheme of beginning with simpleclient.

moved to simpleclient_tracer

brian-brazil · 2021-04-21T07:49:08Z

simpleclient/src/main/java/io/prometheus/client/exemplars/CounterExemplarSampler.java

+
+  /**
+   * @param increment     the value added to the counter on this event
+   * @param newTotalValue the new total counter value after adding the increment.


This is still here, it should be removed.

ok, I removed it

brian-brazil · 2021-04-21T08:00:49Z

simpleclient/src/main/java/io/prometheus/client/exemplars/HistogramExemplarSampler.java

+  /**
+   * @param value      the value to be observed.
+   * @param bucketFrom upper boundary of the previous bucket in the histogram.
+   *                   May be {@link Double#NEGATIVE_INFINITY} if there is no previous bucket.


Will be

There's no uncertainty here.

brian-brazil · 2021-04-21T08:02:40Z

README.md

+
+### Enabling And Disabling Exemplars
+
+By default, Exemplars are enabled if OpenTelemetry tracing is detected. You can disable this globally with:


Does this need updating?

I re-wrote the exemplars documentation.

brian-brazil · 2021-04-21T08:04:26Z

simpleclient/src/main/java/io/prometheus/client/exemplars/ExemplarConfig.java

+  private static final ExemplarConfig instance = new ExemplarConfig();
+
+  private final ExemplarSampler noopExemplarSampler;
+  private final ExemplarSampler defaultExemplarSampler;


Shouldn't these be static? They aren't tied to an instance.

Made them static.

brian-brazil · 2021-04-21T08:05:45Z

simpleclient/src/main/java/io/prometheus/client/exemplars/ExemplarConfig.java

+  }
+
+  /**
+   * Disable exemplars by default. Exemplars can still be enabled for individual metrics in the metric's builder.


Does this actually work? By the time it is called, static initialisation of metrics will likely already be done.

I added the following to README.md to clarify this

Note that ExemplarConfig is used when a metric is created, so changing the default has no effect on metrics that have already been created.

If we want API to change the ExemplarSampler for existing metrics, we'd need to make access to the ExemplarSampler thread safe, which would create unexpected synchronization in multi threaded user applications. I don't see a good way of doing this apart from requiring the user to call ExemplarConfig before metrics are created.

fstab · 2021-05-09T21:39:32Z

Thanks again for your review.

Apart from addressing your comments, I added ...withExemplar() versions for the Counter's inc() and the Histogram's observe() and time() methods. Now users can now control Exemplars on three levels:

Globally by setting the default exemplar sampler
Per metric by setting an exemplar sampler in the metric builder
Per observation by using the ...withExemplar() methods

I described this in README.md. I also split the documentation so that generic Exemplar topics are covered in README.md and OpenTelemetry specific topics in OTEL_EXEMPLARS.md.

I did not find the time yet to re-run all benchmarks, I will do that before the merge.

I tested all combinations of available / unavailable dependencies and all available OpenTelemetry Java versions. I didn't find any combination that breaks anything.

If the OpenTelemetry tracer is detected, Exemplars for OpenTelemetry are enabled by default. I added a few options to disable this

In code by calling ExemplarConfig.disableExemplarSamplers()
At build time by excluding the tracer_otel and tracer_otel_agent dependency
At runtime with system properties or environment variables.

I documented this in OTEL_EXEMPLARS.md.

I like the idea that if users attach the OpenTelemetry Java agent to get traces, they will also see Exemplars by default. And I don't see why people would use distributed tracing with OpenTelemetry without having corresponding exemplars in their metrics. However, if you think that we should not enable exemplars by default if OpenTelemetry tracing is present, I am happy to turn this around and disable OpenTelemetry exemplars by default.

brian-brazil · 2021-05-09T22:32:12Z

README.md

+The following call will increment a counter, and create an exemplar with the specified `span_id` and `trace_id` labels:
+
+```java
+myCounter.incWithExemplar(Exemplar.SPAN_ID, "abcdef", Exemplar.TRACE_ID, "123456");


I'd include the literal string here, rather than pointing off to an unspecified class that you have to import to get to a constant.

Removed the TRACE_ID String from Exemplar.

Did you forget to push? I'm not seeing a change here.

brian-brazil · 2021-05-09T22:32:31Z

README.md

+myHistogram.observeWithExemplar(0.12, Exemplar.SPAN_ID, "abcdef", Exemplar.TRACE_ID, "123456");
+```
+
+All methods for observing and incrementing values have `...withExemplar` equivalents. There are versions taking the exemplar labels as a `String...` as shown in the example, as well as versions taking the exemplar labels as a `Map<String, String>`.


Have you thought about doing this as part of inc() call? This isn't Go, function overloading is possible and normal in Java.

We cannot make it part of the inc() call if we want to use varargs for the labels: inc(3) adds 3 without explicitly specifying an exemplar, while incWithExemplar(3) adds 3 and creates an exemplar without labels.

If someone wanted an explicitly empty exemplar, they could pass an empty map.

brian-brazil · 2021-05-09T22:33:03Z

benchmark/src/main/java/io/prometheus/benchmark/ExemplarsBenchmark.java

@@ -0,0 +1,80 @@
+package io.prometheus.benchmark;


This comment still stands.

brian-brazil · 2021-05-09T22:33:55Z

integration_tests/exemplars_otel/pom.xml

+    <dependency>
+      <groupId>io.opentelemetry</groupId>
+      <artifactId>opentelemetry-api</artifactId>
+      <version>${otel.version}</version>


My comment stands, you can't assume that users are using maven.

brian-brazil · 2021-05-09T22:40:33Z

simpleclient/src/main/java/io/prometheus/client/exemplars/Exemplar.java

+  private final double value;
+  private final Long timestampMs;
+
+  private static final Pattern labelNameRegex = Pattern.compile("[a-zA-Z_]\\w*"); // \w is [a-zA-Z_0-9]


This comment only confuses matters, if you want to say [a-zA-Z_0-9] then say [a-zA-Z_0-9] directly.

Renamed io.prometheus.benchmark to io.prometheus.client.benchmark

Regarding Maven variables: I tested the current build with the oldest 3.0 and 3.1 versions available on the Apache mirror (3.0.5 from 2013-02-23 and 3.1.1 from 2013-10-04). I had to downgrade the failsafe plugin, but apart from that the build runs without issues with these versions. Does that address your concerns?

Changed the regex.

Does that address your concerns?

No, the problem was never maven. It was some other Java build tool that was reading the poms.

brian-brazil · 2021-05-09T22:42:10Z

simpleclient/src/main/java/io/prometheus/client/exemplars/ExemplarConfig.java

+   * or they can be provided explicitly for individual observations with the {@code ...withExemplar()} methods.
+   */
+  public static void disableExemplarSamplers() {
+    enabled = false;


This is still racey, you don't know when this could be called relative to when Counters and Histograms are instantiated.

I added a line to the JavaDoc saying that this must be called before a Counter or Histogram is created.

I'm not sure what the best trade-off is. From a usability perspective it would be better if this disabled Exemplars also for previously created Counter or Histogram metrics. However, in order to implement this we would need to make access to the exemplar sampler thread safe, which might severely slow down multi-threaded use of the metrics. Therefore I think the best option is to require the user to set the default exemplar behavior before creating metrics. WDYT?

I added a line to the JavaDoc saying that this must be called before a Counter or Histogram is created.

That's not possible for users to do, as metrics are usually defined statically and thus users have no control over it.

However, in order to implement this we would need to make access to the exemplar sampler thread safe, which might severely slow down multi-threaded use of the metrics.

If you only want to read the current state of the sampler, that should always be fast as you don't need mutexes. It's only when mixing reads and writes that things can get a tad slow.

brian-brazil · 2021-05-09T22:44:35Z

simpleclient_common/src/test/java/io/prometheus/client/exporter/common/ExemplarTest.java

+    }
+  }
+
+  private void assertOldFormat(String line) throws IOException {


Old and New here is a bit confusing, I'd suggest using their actual names.

Renamed it.

brian-brazil · 2021-05-09T22:46:00Z

simpleclient_common/src/test/java/io/prometheus/client/exporter/common/ExemplarTest.java

+        metrics.contains("\n" + line));
+  }
+
+  private String timestampString() {


Why are you creating a copy of this function in the tests?

Removed the copy and used the version from TextFormat.

brian-brazil

I think you forgot to push.

brian-brazil · 2021-05-12T07:35:09Z

README.md

+The following call will increment a counter, and create an exemplar with the specified `span_id` and `trace_id` labels:
+
+```java
+myCounter.incWithExemplar(Exemplar.SPAN_ID, "abcdef", Exemplar.TRACE_ID, "123456");


Did you forget to push? I'm not seeing a change here.

fstab · 2021-05-12T15:12:35Z

Sorry, I removed the reference to Exemplar.SPAN_ID from all code occurrences, but missed the one in README.md.

If you only want to read the current state of the sampler, that should always be fast as you don't need mutexes. It's only when mixing reads and writes that things can get a tad slow.

Ok, then I'll refactor this such that you can change the exemplar sampler for existing metrics.

fstab · 2021-05-13T23:04:32Z

I pushed another update. Now changes in ExemplarConfig will be picked up by existing Counters and Histograms.
I also ran all benchmarks again, as this was still on my todo list.

Regarding the variables in pom.xml files I think it's fair to require users to use Maven or the ./mvnw script to build the project from source. I'm not aware of an alternative build tool that uses the pom.xml files. However, if you think there are users out there using such a tool I am happy to replace the variables with the version numbers.

brian-brazil

This looks roughly right.

However, if you think there are users out there using such a tool I am happy to replace the variables with the version numbers.

As I said it's a complaint I got years ago, for which I forget the exact details. It's your call.

brian-brazil · 2021-05-14T07:00:49Z

README.md

+
+### Per Observation Exemplars
+
+You can explicitly create an exemplar for an individual observation. This takes precedence over the exemplar sampler configured with the metric.


s/create/provide/

brian-brazil · 2021-05-14T07:02:10Z

integration_tests/exemplars_otel_agent/src/test/resources/Dockerfile

@@ -0,0 +1,10 @@
+FROM openjdk:11-jre
+RUN mkdir -p /app/
+ADD https://github.com/open-telemetry/opentelemetry-java-instrumentation/releases/download/v1.1.0/opentelemetry-javaagent-all.jar /app/


Seems that the agent release lags a few days behind the library release. Agent is still 1.1.0.

brian-brazil · 2021-05-14T07:03:37Z

integration_tests/exemplars_otel/pom.xml

+      <artifactId>simpleclient</artifactId>
+      <version>${project.version}</version>
+      <exclusions>
+        <!-- uncomment the following to verify manually that simpleclient still works if the tracers are excluded -->


Could this be another integration test?

Good idea, I turned this into an integration test. Instead of excluding dependencies in the pom.xml, the new implementation of ExemplarsOtelIT copies the individual JARs into the Docker image so that they can be removed in the test without rebuilding the Maven project.

While I was at it, I also renamed the tracer_... sub-modules to simpleclient_tracer_....

brian-brazil · 2021-05-14T07:05:48Z

simpleclient/src/main/java/io/prometheus/client/Counter.java

+    /**
+     * Disable exemplars.
+     * <p>
+     * This will disable exemplars for this counter even if exemplars are enabled by default in {@link ExemplarConfig}.


This reads as though it prevents all use of exemplars, whereas it's only the automatic ones that it stops. This could be clearer.

Changed the wording.

brian-brazil · 2021-05-14T07:09:48Z

simpleclient_common/src/test/java/io/prometheus/client/exporter/common/ExemplarTest.java

+    assert004Format("no_labels_default_exemplar_total 3.0\n");
+    noLabelsDefaultExemplar.inc(2);
+    // The TestExemplarSampler always produces a new Exemplar.
+    // The Exemplar with value 3.0 should be replaced with an Exemplar with value 5.0.


This comment isn't right, the exemplar value is 2. It's the counter value that is 5.

brian-brazil · 2021-05-14T07:19:25Z

benchmark/README.md

-    i.p.b.CounterBenchmark.prometheusSimpleCounterNoLabelsIncBenchmark    avgt        5   13.131 ± 1.661  ns/op
+    i.p.c.b.CounterBenchmark.codahaleCounterIncBenchmark                    avgt        5   8.063 ± 0.440  ns/op
+    i.p.c.b.CounterBenchmark.codahaleMeterMarkBenchmark                     avgt        5  38.625 ± 2.111  ns/op
+    i.p.c.b.CounterBenchmark.prometheusCounterChildIncBenchmark             avgt        5   9.869 ± 0.252  ns/op


Incidentally, we can remove these old Java client benchmarks. They've not been relevant in many years.

Do you mean remove from README.md, or remove the implementations as well? Are there benchmarks that are still relevant apart from the ExemplarsBenchmark?

The implementations for the old java client. The benchmarks are still relevant, that client isn't however.

Ok, I removed the old client from the benchmarks.

fstab · 2021-05-15T20:41:32Z

I removed the obsolete benchmarks, and I bumped the OpenTelemetry agent version as 1.2.0 was released today. What do you think?

brian-brazil

Gave it a deeper review, and found a few nits.

brian-brazil · 2021-05-17T09:59:07Z

OTEL_EXEMPLARS.md

+request_duration_histogram_bucket{path="/god-of-fire",le="0.004"} 4.0 # {trace_id="043cd631811e373e4180a678c06b128e",span_id="cd122e457d2ca5b0"} 0.0033 1618261159.027
+```
+
+Note that this is an example application for a unit test, so durations don't represent real durations, and some example metrics might not make sense in the real world.


s/unit test/demonstration/

brian-brazil · 2021-05-17T09:59:37Z

OTEL_EXEMPLARS.md

+
+## Disabling OpenTelemetry Exemplars
+
+If you use OpenTelemetry tracing but don't want Exemplars, you can disable OpenTelemetries in multiple ways.


do not

Contractions are harder to deal with for non-native speakers apparently.

brian-brazil · 2021-05-17T10:01:29Z

OTEL_EXEMPLARS.md

+### Disable OpenTelemetry Exemplars at Runtime
+
+If your application uses OpenTelemetry tracing, but you want to disable OpenTelemetry at runtime without changing code,
+start your application with the `otelExemplars` system property:


This should probably be namespaced a bit more, so that it's tied to this client. As-is I'd presume from the name that it was an OTel internal.

Prefixed with io.prometheus.

brian-brazil · 2021-05-17T10:02:27Z

OTEL_EXEMPLARS.md

+java -DotelExemplars=inactive -jar my-application.jar
+```
+
+Alternatively, you can set the environment variable `OTEL_EXEMPLARS=inactive`:


I'd recommend sticking with one way of doing things to avoid confusion. System properties are the way to do this in Java.

Removed the environment variable.

brian-brazil · 2021-05-17T10:13:18Z

...c/main/java/io/prometheus/client/exemplars/tracer/otel/OpenTelemetrySpanContextSupplier.java

+      }
+      OpenTelemetrySpanContextSupplier test = new OpenTelemetrySpanContextSupplier();
+      test.getSpanId();
+      test.getSpanId();


Should this be getTraceId?

Yes, good point. Fixed.

brian-brazil · 2021-05-17T10:13:56Z

.../io/prometheus/client/exemplars/tracer/otel_agent/OpenTelemetryAgentSpanContextSupplier.java

+      }
+      OpenTelemetryAgentSpanContextSupplier test = new OpenTelemetryAgentSpanContextSupplier();
+      test.getSpanId();
+      test.getSpanId();


fstab · 2021-05-17T21:05:52Z

Thanks a lot for reviewing again. I pushed another update. What do you think?

brian-brazil · 2021-05-18T09:52:36Z

👍

fstab · 2021-05-24T20:04:49Z

Thanks a lot for your great review!

fstab mentioned this pull request Apr 12, 2021

Add Exemplar Support to Client Java #622

Closed

brian-brazil reviewed Apr 12, 2021

View reviewed changes

fstab force-pushed the otel-exemplars branch 2 times, most recently from 0244904 to ef6e57f Compare April 18, 2021 22:54

brian-brazil reviewed Apr 19, 2021

View reviewed changes

fstab force-pushed the otel-exemplars branch from b41384f to 126b32b Compare April 19, 2021 22:24

brian-brazil reviewed Apr 21, 2021

View reviewed changes

fstab force-pushed the otel-exemplars branch 2 times, most recently from 650aa32 to 64c7c2b Compare May 9, 2021 21:10

fstab force-pushed the otel-exemplars branch from 64c7c2b to d8fbf66 Compare May 9, 2021 21:39

brian-brazil reviewed May 9, 2021

View reviewed changes

fstab force-pushed the otel-exemplars branch from 3f389ae to 8ce5a53 Compare May 11, 2021 21:15

brian-brazil reviewed May 12, 2021

View reviewed changes

brian-brazil reviewed May 14, 2021

View reviewed changes

brian-brazil reviewed May 17, 2021

View reviewed changes

fstab force-pushed the otel-exemplars branch 2 times, most recently from 602a5a0 to 603a9df Compare May 18, 2021 20:53

add Exemplar support for OpenTelemetry tracing

c4f8b5c

fstab force-pushed the otel-exemplars branch from 603a9df to c4f8b5c Compare May 24, 2021 19:48

fix flaky JettyStatisticsCollectorTest

7490adc

fstab changed the base branch from master to next-release May 24, 2021 20:03

fstab merged commit d216b3f into next-release May 24, 2021


		### Enabling And Disabling Exemplars

		By default, Exemplars are enabled if OpenTelemetry tracing is detected. You can disable this globally with:


		### Per Observation Exemplars

		You can explicitly create an exemplar for an individual observation. This takes precedence over the exemplar sampler configured with the metric.


		## Disabling OpenTelemetry Exemplars

		If you use OpenTelemetry tracing but don't want Exemplars, you can disable OpenTelemetries in multiple ways.

add Exemplar support for OpenTelemetry tracing #652

add Exemplar support for OpenTelemetry tracing #652

Conversation

fstab commented Apr 12, 2021 • edited Loading

New Section in README:

Running an Exemplars Example

Enabling And Disabling Exemplars

Implement Your Own Exemplar Sampling Algorithm

Implement Support for Other Tracing Vendors

Implementation Notes

brian-brazil left a 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

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

fstab commented Apr 18, 2021

brian-brazil left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fstab commented Apr 20, 2021 • edited Loading

brian-brazil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fstab May 4, 2021 • edited Loading

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

fstab May 9, 2021 • edited Loading

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

fstab commented May 9, 2021 • edited Loading

Choose a reason for hiding this comment

fstab commented Apr 12, 2021 •

edited

Loading

fstab commented Apr 20, 2021 •

edited

Loading

fstab May 4, 2021 •

edited

Loading

fstab May 9, 2021 •

edited

Loading

fstab commented May 9, 2021 •

edited

Loading