APM Metering API #99832

stu-elastic · 2023-09-22T20:19:48Z

Adds Metering instrument interfaces and adapter implementations for opentelemetry instrument types:

Gauge - a single number that can go up or down
Histogram - bucketed samples
Counter - monotonically increasing summed value
UpDownCounter - summed value that may decrease

Supports both Long* and Double* versions of the instruments.

Instruments can be registered and retrieved by name through APMMeter which is available via the APMTelemetryProvider.

The metering provider starts as the open telemetry noop provider.

Adds Metering instrument interfaces and adapter implementations for opentelemetry instrument types: * Gauge - a signle number that can go up or down * Histogram - bucketed samples * Counter - monotonically increaming summed value * UpDownCounter - summed value that may decrease Supports both Long* and Double* versions of the instruments. Instruments can be registered and retrieved by MetricName through APMMeter which is available via the APMTelemetryProvider. The metering provider starts as the open telemetry noop provider.

elasticsearchmachine · 2023-09-22T20:20:12Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

elasticsearchmachine · 2023-09-22T20:20:35Z

Hi @stu-elastic, I've created a changelog YAML for you.

rjernst

Overall this looks good. I'm really pleased with the relative simplicity of the api, and am happy you were able to isolate the implementation.

I left some thoughts. In general the main things to look at are adding javadocs and thinking through some of the terminology (it was a bit confusing at times while reading through).

rjernst · 2023-09-23T19:39:14Z

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/APMMetric.java

+
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.APM_ENABLED_SETTING;
+
+public class APMMetric extends AbstractLifecycleComponent implements org.elasticsearch.telemetry.metric.Metric {


nit: Would APMMeter be a better name? It matches more with "APMTracer" IMO, and also matches the underly otel terminology, while metric sounds to me like a single datapoint.

++ on Meter to match OTEL terminology

Changed to APMMeter.

rjernst · 2023-09-23T19:41:18Z

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/APMMetric.java

+        this.enabled = APM_ENABLED_SETTING.get(settings);
+    }
+
+    public void setEnabled(boolean enabled) {


Where is this called? Doesn't it need to be wired up to the dynamic enabled setting? Also, are there any concurrency considerations then?

it is here wired to dynamic cluster settings
https://github.com/elastic/elasticsearch/pull/99832/files#diff-8e5ca1315006cc00c58ef42d4bfc14f50b30d11567a845f8e551ecf3e52c830bR58
regarding concurrency considerations we concluded that:

enabled should be volatile

Instruments.setProvider method is acquiring a lock. It needs this so that new instruments are not added at the same time when the implementation of the Meter (noop/otel) is set

whenever we add a new instrument via register* methods we need to acquire a lock
adding new instruments calling setProvider should be rare (we expect those to be called at most once during node lifetime)

rjernst · 2023-09-23T19:43:12Z

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/APMMetric.java

+
+public class APMMetric extends AbstractLifecycleComponent implements org.elasticsearch.telemetry.metric.Metric {
+    private static final Meter NOOP = OpenTelemetry.noop().getMeter("noop");
+    private final Instruments instruments = new Instruments(NOOP);


Should this start with the correct instrument if enabled on construction?

rjernst · 2023-09-23T19:46:46Z

server/src/main/java/org/elasticsearch/telemetry/metric/MetricName.java

+import java.util.Objects;
+
+public class MetricName {
+    public static final MetricName EMPTY = new MetricName("EMPTY");


In what case do we need an empty metric name?

NOOP meter from the TelemetryProvider. Currently used by default in a bunch of tests that don't care about metrics.

Removed MetricName and changed these names to noop.

rjernst · 2023-09-23T19:47:38Z

server/src/main/java/org/elasticsearch/telemetry/metric/MetricName.java

+
+import java.util.Objects;
+
+public class MetricName {


This class seems like a simple wrapper around a String name. Any reason we can't just use String, or at worse a record?

It was to make it easier to find metric names in the code base based on inspiration of TracerId but we can start with a String and refactor if necessary.

in this case we should remove MetricName class? I cannot see if it used.
in https://github.com/elastic/elasticsearch/pull/99832/files#r1336150731 you mention you removed, perhaps you missed to commit it

rjernst · 2023-09-23T19:49:19Z

server/src/main/java/org/elasticsearch/telemetry/metric/Metric.java

+
+package org.elasticsearch.telemetry.metric;
+
+public interface Metric {


Javadocs on this class and methods will help other devs learn how to use the api.

rjernst · 2023-09-23T19:51:42Z

server/src/main/java/org/elasticsearch/telemetry/metric/LongUpDownCounter.java

+import java.util.Map;
+
+public interface LongUpDownCounter extends Instrument {
+    void add(long inc);


It's not clear to me when one would be expected to use these 3 variants of add. Javadocs would help.

Removed the threadcontext version and added javadocs.

rjernst · 2023-09-23T20:01:01Z

...src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/LongUpDownCounterAdapter.java

+
+    @Override
+    LongUpDownCounter buildInstrument(Meter meter) {
+        return Objects.requireNonNull(meter).upDownCounterBuilder(getName()).setDescription(getDescription()).setUnit(getUnit()).build();


nit: consider making a local var for the builder so this is not such a long line (difficult to read).

Added in all the adapters will long lines.

rjernst · 2023-09-23T20:01:03Z

server/src/main/java/org/elasticsearch/telemetry/metric/LongUpDownCounter.java

+import java.util.Map;
+
+public interface LongUpDownCounter extends Instrument {
+    void add(long inc);


It's not clear to me when one would be expected to use these 3 variants of add. Javadocs would help.

Removed the threadcontext version and added javadocs.

rjernst · 2023-09-23T21:29:07Z

...s/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/AbstractInstrument.java

+import java.util.Objects;
+import java.util.concurrent.atomic.AtomicReference;
+
+public abstract class AbstractInstrument<T, I> implements Instrument {


The terminology here is a bit confusing. This is an instrument, but it also has an instrument?

I think this is more like a delegate/decorator pattern.
One role of this class is to replace the implementation of the underlying delegate instrument. Perhaps we could rename this?
Brainstorming some ideas for a name: SwitchableInstrument , AbstractSwitchableInstrument (I am biased on my previous draft)

But is also has buildInstrument which fits the simple AbstractInstrument too.

alternatively we could just rename instrumentRef to delegate. Although I am not sure how much it helps

pgomulka

looks great, left comments

pgomulka · 2023-09-25T07:26:59Z

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/APMMetric.java

+
+import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.APM_ENABLED_SETTING;
+
+public class APMMetric extends AbstractLifecycleComponent implements org.elasticsearch.telemetry.metric.Metric {


++ on Meter to match OTEL terminology

pgomulka · 2023-09-25T07:29:54Z

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/APMMetric.java

+    private static final Meter NOOP = OpenTelemetry.noop().getMeter("noop");
+    private final Instruments instruments = new Instruments(NOOP);
+    private volatile boolean enabled;
+    private final AtomicReference<APMServices> services = new AtomicReference<>();


we don't use services in this implementation. All we need is instruments right?

pgomulka · 2023-09-25T07:33:20Z

...s/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/AbstractInstrument.java

+import java.util.Objects;
+import java.util.concurrent.atomic.AtomicReference;
+
+public abstract class AbstractInstrument<T, I> implements Instrument {


I think this is more like a delegate/decorator pattern.
One role of this class is to replace the implementation of the underlying delegate instrument. Perhaps we could rename this?
Brainstorming some ideas for a name: SwitchableInstrument , AbstractSwitchableInstrument (I am biased on my previous draft)

But is also has buildInstrument which fits the simple AbstractInstrument too.

alternatively we could just rename instrumentRef to delegate. Although I am not sure how much it helps

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/Instruments.java

pgomulka

2 comments.
I think we should also add some tests (I will look into this and try to commit to this PR)
alternatively the API (interfaces only) could be split from this PR and merged. And we could iterate on the implementation and keep on adding tests/improvements

pgomulka · 2023-09-25T10:11:51Z

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/APMMetric.java

+    record APMServices(Meter meter, OpenTelemetry openTelemetry) {}
+
+    public APMMetric(Settings settings) {
+        this.enabled = APM_ENABLED_SETTING.get(settings);


We should call setEnabled here. currently a default value for tracing.apm.enabled is false, so there will always be a cluster state update event that will update the value to true.
However, if we at some point change the default to true, there will be no setting value change, so setEnabled will never be called from cluster settings updater https://github.com/elastic/elasticsearch/blob/main/modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/APMAgentSettings.java#L45

I think we can mark the class final, so that we can safely call setEnabled (it is a public method, so there is a risk of this escape problem if someone extend this class).

pgomulka · 2023-09-25T10:16:08Z

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/APMMetric.java

+        this.enabled = APM_ENABLED_SETTING.get(settings);
+    }
+
+    public void setEnabled(boolean enabled) {


it is here wired to dynamic cluster settings
https://github.com/elastic/elasticsearch/pull/99832/files#diff-8e5ca1315006cc00c58ef42d4bfc14f50b30d11567a845f8e551ecf3e52c830bR58
regarding concurrency considerations we concluded that:

enabled should be volatile

Instruments.setProvider method is acquiring a lock. It needs this so that new instruments are not added at the same time when the implementation of the Meter (noop/otel) is set

whenever we add a new instrument via register* methods we need to acquire a lock
adding new instruments calling setProvider should be rare (we expect those to be called at most once during node lifetime)

felixbarny · 2023-09-25T11:58:41Z

Great to see metric infrastructure being added to Elasticsearch!

Just a drive-by question out of curiosity. Why is this abstracting/wrapping the OpenTelemetry Metric API into an Elasticsearch-specific metrics API instead of directly providing access to the OpenTelemetry Metric API?

pgomulka · 2023-09-25T12:47:29Z

Why is this abstracting/wrapping the OpenTelemetry Metric API into an Elasticsearch-specific metrics API instead of directly providing access to the OpenTelemetry Metric API?

@felixbarny Using OTEL api directly would require introducing this as a dependency to all the modules/plugins that want to use it. With metrics (but tracing too) we also want to have control when the feature is enabled (hence the 'switcheable' abstract implementation)
More generally abstracting the 3rd party library allows us to hide the implementation and refactor if we ever need this.

pgomulka · 2023-09-25T13:50:01Z

...s/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/AbstractInstrument.java

+    private final T unit;
+
+    public AbstractInstrument(Meter meter, MetricName name, String description, T unit) {
+        this.instrumentRef = new AtomicReference<>(buildInstrument(meter));


there is a problem with this approach.

buildInstrument implementations are relying on the getName, getDescription and getUnit methods. THose are not initialised yet.

the implementing classes might implement this 'in a wrong way' (unlikely but still) leading to concurrency problems. Perhaps we can mitigate this by sealed class.

I think we should change the signature of buildInstrument method to accept name, description and unit and we could pass it as a constructor parameter

Discussed offline, moved buildInstrument last to avoid other fields being uninitialized.

…ractInstrument

… into meter-noop-api

…/elasticsearch into meter-noop-api

pgomulka

looks good, I will address comments from this review

pgomulka · 2023-09-26T08:45:14Z

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/APMAgentSettings.java

@@ -159,7 +167,7 @@ public void setAgentSetting(String key, String value) {

    public static final Setting<Boolean> APM_ENABLED_SETTING = Setting.boolSetting(
        APM_SETTING_PREFIX + "enabled",
-        false,
+        true,


we should switch this back to false before we merge and override this in serverless

Thanks for adding a commit to do this.

pgomulka · 2023-09-26T08:46:26Z

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/APMMeter.java

+        return provider.get();
+    }
+
+    private void destroyApmServices() {


we could refactor this method to return noop instance.
(same as above createApmServices)

Thanks for adding a commit to do this.

pgomulka · 2023-09-26T08:46:56Z

modules/apm/src/main/java/org/elasticsearch/telemetry/apm/internal/metrics/APMMeter.java

+        this.otelMeterSupplier = otelMeterSupplier;
+        this.noopMeterSupplier = noopMeterSupplier;
+        this.instruments = new Instruments(enabled ? createApmServices() : noopMeterSupplier.get());
+        setEnabled(enabled);


given the above line, we no longer have to call setEnabled

Thanks for adding a commit to do this.

pgomulka · 2023-09-26T08:49:16Z

server/src/main/java/org/elasticsearch/telemetry/metric/MetricName.java

+
+import java.util.Objects;
+
+public class MetricName {


in this case we should remove MetricName class? I cannot see if it used.
in https://github.com/elastic/elasticsearch/pull/99832/files#r1336150731 you mention you removed, perhaps you missed to commit it

ldematte · 2023-09-27T15:29:38Z

It was quite a long PR to go through but worth it. I really like how linear the API is, and how the OTel implementation is hidden effectively. I personally don't like abstract classes like AbstractInstrument (I like a very flat hierarchy) but it's a matter of personal taste and I can see why it is very convenient here.
Looks GREAT to me 👍

…op-api

The latest version contains a fix to allow sending metrics to APM server. also adds a apm agent jvm options "enable_experimental_instrumentations", "true" which is required to enable the otel-metrics-instrumentation. relates #99832

Adds Metering instrument interfaces and adapter implementations for opentelemetry instrument types: * Gauge - a single number that can go up or down * Histogram - bucketed samples * Counter - monotonically increasing summed value * UpDownCounter - summed value that may decrease Supports both Long* and Double* versions of the instruments. Instruments can be registered and retrieved by name through APMMeter which is available via the APMTelemetryProvider. The metering provider starts as the open telemetry noop provider. `telemetry.metrics.enabled` turns on metering.

The latest version contains a fix to allow sending metrics to APM server. also adds a apm agent jvm options "enable_experimental_instrumentations", "true" which is required to enable the otel-metrics-instrumentation. relates elastic#99832

Adds Metering instrument interfaces and adapter implementations for opentelemetry instrument types: * Gauge - a single number that can go up or down * Histogram - bucketed samples * Counter - monotonically increasing summed value * UpDownCounter - summed value that may decrease Supports both Long* and Double* versions of the instruments. Instruments can be registered and retrieved by name through APMMeter which is available via the APMTelemetryProvider. The metering provider starts as the open telemetry noop provider. `telemetry.metrics.enabled` turns on metering.

The latest version contains a fix to allow sending metrics to APM server. also adds a apm agent jvm options "enable_experimental_instrumentations", "true" which is required to enable the otel-metrics-instrumentation. relates elastic#99832

stu-elastic added >enhancement :Core/Infra/Core Core issues without another label labels Sep 22, 2023

stu-elastic requested review from rjernst and pgomulka September 22, 2023 20:19

elasticsearchmachine added v8.11.0 Team:Core/Infra Meta label for core/infra team labels Sep 22, 2023

Update docs/changelog/99832.yaml

1c55747

stu-elastic mentioned this pull request Sep 22, 2023

Metering API Implementation #99723

Closed

rjernst reviewed Sep 23, 2023

View reviewed changes

pgomulka reviewed Sep 25, 2023

View reviewed changes

stu-elastic and others added 11 commits September 25, 2023 09:31

depegate apm service creation, fix intstrument creation order in Abst…

04189b4

…ractInstrument

3tests

47faa2e

APMMetric -> APMMeter

de9b4aa

Merge branch 'meter-noop-api' of github.com:stu-elastic/elasticsearch…

fd70837

… into meter-noop-api

Metric -> Meter, MetricName -> String

3a295fb

Add builder var in buildInstrument

f5ddd70

Create instrument with provider if enabled on construction

d931b13

javadocs, remove type param for unit, remove thread context version

9eefc0b

Merge branch 'test_stu-elastic-meter-noop-api' of github.com:pgomulka…

4a3d95f

…/elasticsearch into meter-noop-api

pull Przemek's tests and refactor

0d76486

Remove Meter parameterization, remove @test annotation, add javadocs

3c40aa6

pgomulka reviewed Sep 26, 2023

View reviewed changes

tests, setup in apmmeter

dbf00ab

pgomulka added 2 commits September 26, 2023 14:12

Merge remote-tracking branch 'upstream/main' into meter-noop-api

d063ca4

disable apm by default

2b43a35

pgomulka approved these changes Sep 26, 2023

View reviewed changes

fix security manager exceptions

b3850da

stu-elastic added 9 commits September 28, 2023 11:27

Add apm.metrics.enabled setting

8513bc6

Merge branch 'main' of github.com:elastic/elasticsearch into meter-no…

6ade418

…op-api

Merge branch 'main' of github.com:elastic/elasticsearch into meter-no…

7ba6ac2

…op-api

apm.tracing.metrics.enabled -> telemetry.metrics.enabled

5f130d4

use telemetry.metrics.enabled setting in APMMeterTests

774a41d

Merge branch 'main' of github.com:elastic/elasticsearch into meter-no…

ffaf4ef

…op-api

APM_METRICS_ENABLED_SETTING -> TELEMETRY_METRICS_ENABLED_SETTING

bbd11de

use method ref in createOtelMeter

5df2546

remove getServices in APMTracer

c9f7d29

stu-elastic merged commit f8d09e9 into elastic:main Sep 29, 2023

pgomulka mentioned this pull request Sep 29, 2023

Update the elastic-apm-agent version #100064

Merged


		import static org.elasticsearch.telemetry.apm.internal.APMAgentSettings.APM_ENABLED_SETTING;

		public class APMMetric extends AbstractLifecycleComponent implements org.elasticsearch.telemetry.metric.Metric {


		package org.elasticsearch.telemetry.metric;

		public interface Metric {

APM Metering API #99832

APM Metering API #99832

Conversation

stu-elastic commented Sep 22, 2023 • edited Loading

elasticsearchmachine commented Sep 22, 2023

elasticsearchmachine commented Sep 22, 2023

rjernst 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

stu-elastic Sep 25, 2023 • 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

Choose a reason for hiding this comment

pgomulka 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

pgomulka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixbarny commented Sep 25, 2023

pgomulka commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka 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

ldematte commented Sep 27, 2023

stu-elastic commented Sep 22, 2023 •

edited

Loading

stu-elastic Sep 25, 2023 •

edited

Loading