Record HTTP server metrics in instrumenter API. #2877

anuraaga · 2021-04-27T08:49:52Z

Initial draft of recording metrics from Instrumenter. Let me know if you have any starter thoughts on this approach.

I found recording the metrics using the metrics API to be pretty annoying, especially having different set of labels for different metrics. I'm hoping the metrics conventions are moving in a direction of recording all the attributes, and letting the SDK deal with such details. That being said, it's still nice that all this is hidden from instrumentation authors, they just have to enable the metrics when constructing.

anuraaga · 2021-04-27T08:55:57Z

...tion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/RequestMetrics.java

+
+  Context start(Context context, Attributes requestAttributes);
+
+  void end(Context context, Attributes responseAttributes);


My understanding is metrics will only ever be recorded with "request attributes" since metric attributes (labels) must be constant throughout a request.

But some metrics will need response attributes to get the actual value, for example, a "response content length" metric. In other words, some span attributes (usually response attributes) are actually metric values.

anuraaga · 2021-04-27T08:57:34Z

...i/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/RequestMetricsFactory.java

+import io.opentelemetry.api.metrics.Meter;
+
+@FunctionalInterface
+public interface RequestMetricsFactory {


Possibly this class could be called RequestMetrics and the other RequestListener

Note: if we ever want to implement this comment: we could encapsulate all span operations in a RequestListener

Yeah that seems like a nice idea

jkwatson · 2021-04-27T15:02:30Z

...rc/main/java/io/opentelemetry/instrumentation/api/instrumenter/HttpServerRequestMetrics.java

+    activeRequests =
+        meter
+            .longUpDownCounterBuilder("http.server.active_requests")
+            .setUnit("requests")


This makes me think the semantic conventions need to have units in them as well, so we won't have to hard-code this stuff. Also, even though this is currently the semantic convention for these units, I suspect this will not last, as we will probably be standardizing on units in the future. See this issue for tracking: open-telemetry/opentelemetry-specification#705

Yeah the conventions do have units, hence me setting a strange unit here :P

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-server

I was thinking the generator should provide them as well. Are they in the yaml yet?

Ah got it - nope there aren't any metric yaml files yet

richardstartin · 2021-04-27T20:48:57Z

...rc/main/java/io/opentelemetry/instrumentation/api/instrumenter/HttpServerRequestMetrics.java

+            case "http.method":
+            case "http.host":
+            case "http.scheme":
+            case "http.status_code":
+            case "http.flavor":
+            case "http.server_name":
+            case "net.host.name":
+            case "net.host.port":


I'm curious about OTel's approach to high cardinality metric attributes, having implemented similar functionality at Datadog and paid a lot of attention to cardinality reduction prior to aggregation. I have a few questions which I hope are not disruptive to your review.

I'm unfamiliar with OTel attributes but which of these, if any, is the request path?

Since the unsanitised request path can have unbounded cardinality, would that rule it out as a metric attribute?

If this were a metric for a database query, would you sanitise the query here, or exclude it from the labels?

Thanks for reaching out @richardstartin. I doubt I'll have good answers as lots about OTel's approach to metrics is WIP but let's see ;)

I'm unfamiliar with OTel attributes but which of these, if any, is the request path?

It's not in this list yet. There is a special note here

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#parameterized-labels

and I didn't want to deal with it yet :) In particular, it seems they are not very up to date as http.route is really the parameter to be using (it's the parameterized path)

Since the unsanitised request path can have unbounded cardinality, would that rule it out as a metric attribute?

I think so, it would have to be a parameterized path of some sort. That being said, given the separation between API and SDK in OTel, I wonder if it's possible to pass raw paths in API and in the SDK either detect parameters in the path using some cool logic, or detect the cardinality and drop the attributes. Without that, it's definitely not appropriate. I wonder if your "cardinality reduction prior to aggregation" would be something we can incorporate in the SDK.

If this were a metric for a database query, would you sanitise the query here, or exclude it from the labels?

If there were conventions for DB metrics I'd just follow them

https://github.com/open-telemetry/opentelemetry-specification/tree/main/specification/metrics/semantic_conventions

Lacking them, just from my sense, I've never added queries as metric labels. But I don't see a problem with adding prepared statements as they generally have low cardinality (10-20 different types of queries in an app, each with the same prepared form across invocations).

Thanks for your answers. We track metrics at a much more granular level than here - so including the sanitised request path, sanitised database query, and so on - we call this the resource name at Datadog. This is because latency distributions tend to depend on attributes like the query or the endpoint - e.g. database updates and inserts tend to perform differently, and the application's business logic is always slower than the health check endpoint.

I will be interested to see the direction this API goes in - will higher cardinality attributes be sacrificed altogether to keep the number of points down, or will it find a way to share the sanitisation required to avoid the number of metric points exploding with tracing to reduce total overhead and promote consistency between metrics and tracing?

instrumentation-api/instrumentation-api.gradle

mateuszrzeszutek · 2021-04-29T13:20:15Z

...i/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/RequestMetricsFactory.java

+import io.opentelemetry.api.metrics.Meter;
+
+@FunctionalInterface
+public interface RequestMetricsFactory {


Note: if we ever want to implement this comment: we could encapsulate all span operations in a RequestListener

mateuszrzeszutek · 2021-04-29T13:22:40Z

...rc/main/java/io/opentelemetry/instrumentation/api/instrumenter/HttpServerRequestMetrics.java

+      return;
+    }
+    activeRequests.add(-1, state.activeRequestLabels());
+    duration.record(System.nanoTime() - state.startTimeNanos(), state.durationLabels());


The metric's unit is milliseconds, we probably should convert nanos to millis

mateuszrzeszutek · 2021-04-29T13:27:18Z

...rc/main/java/io/opentelemetry/instrumentation/api/instrumenter/HttpServerRequestMetrics.java

+    LabelsBuilder labels = Labels.builder();
+    attributes.forEach(
+        (key, value) -> {
+          if (key.getType() != AttributeType.STRING) {


http.status_code and net.host.port are longs, not strings.

Hmm - status code isn't ready anyways when I compute these labels at the beginning of the request. I guess depending on the metric, some need to be computed at beginning (active requests) and some at end (duration). But since there's no text in the spec on that yet I'll keep it as is for now.

mateuszrzeszutek · 2021-04-29T13:29:49Z

...rc/main/java/io/opentelemetry/instrumentation/api/instrumenter/HttpServerRequestMetrics.java

+    LabelsBuilder labels = Labels.builder();
+    attributes.forEach(


It'd be great if the SDK included some easy Attributes<->Labels conversion. Or if we could use Attributes directly (well, a subset of them) in metrics API...

Yeah that's what I'm hoping for.

…nstrumentation into http-request-metrics

anuraaga

Started going through comments will continue to clean this PR up

anuraaga · 2021-05-07T04:17:17Z

...rc/main/java/io/opentelemetry/instrumentation/api/instrumenter/HttpServerRequestMetrics.java

+    LabelsBuilder labels = Labels.builder();
+    attributes.forEach(


Yeah that's what I'm hoping for.

anuraaga · 2021-05-07T04:22:05Z

...rc/main/java/io/opentelemetry/instrumentation/api/instrumenter/HttpServerRequestMetrics.java

+    LabelsBuilder labels = Labels.builder();
+    attributes.forEach(
+        (key, value) -> {
+          if (key.getType() != AttributeType.STRING) {


Hmm - status code isn't ready anyways when I compute these labels at the beginning of the request. I guess depending on the metric, some need to be computed at beginning (active requests) and some at end (duration). But since there's no text in the spec on that yet I'll keep it as is for now.

anuraaga · 2021-05-07T04:22:59Z

...i/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/RequestMetricsFactory.java

+import io.opentelemetry.api.metrics.Meter;
+
+@FunctionalInterface
+public interface RequestMetricsFactory {


Yeah that seems like a nice idea

…nstrumentation into http-request-metrics

anuraaga · 2021-05-10T06:26:10Z

Thanks for waiting think this is ready for review

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

.../src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetrics.java

…nstrumentation into http-request-metrics

mateuszrzeszutek · 2021-05-11T09:13:32Z

...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java

@@ -74,6 +81,12 @@
    return addAttributesExtractors(Arrays.asList(attributesExtractors));
  }

+  /** Adds a {@link RequestMetrics} whose metrics will be recorded for request start and stop. */
+  public InstrumenterBuilder<REQUEST, RESPONSE> addRequestMetrics(RequestMetrics factory) {


Shouldn't it be marked as unstable?

Suggested change

public InstrumenterBuilder<REQUEST, RESPONSE> addRequestMetrics(RequestMetrics factory) {

@UnstableApi

public InstrumenterBuilder<REQUEST, RESPONSE> addRequestMetrics(RequestMetrics factory) {

Apparently I was happy enough having just written the docs on the annotation...

mateuszrzeszutek · 2021-05-11T09:14:53Z

.../src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetrics.java

+ * <p>To use this class, you may need to add the {@code opentelemetry-api-metrics} artifact to your
+ * dependencies.
+ */
+public final class HttpServerMetrics implements RequestListener {


Same here, shouldn't we mark this class as unstable?

Suggested change

public final class HttpServerMetrics implements RequestListener {

@UnstableApi

public final class HttpServerMetrics implements RequestListener {

mateuszrzeszutek · 2021-05-11T10:00:52Z

...rc/main/java/io/opentelemetry/javaagent/instrumentation/okhttp/v3_0/OkHttp3Interceptors.java

@@ -14,4 +14,5 @@

  public static final Interceptor TRACING_INTERCEPTOR =
      OkHttpTracing.create(GlobalOpenTelemetry.get()).newInterceptor();
+  String a = Byte.toString((byte) 2);


To be removed?

mateuszrzeszutek · 2021-05-11T10:02:31Z

...ion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/RequestListener.java

+  Context start(Context context, Attributes attributes);
+
+  /** Listener method that is called at the end of a request. */
+  void end(Context context, Attributes attributes);


Should we mention that end() only receives attributes extracted from the response, without the request ones? This is not immediately obvious when you look at this interface.

Suggested change

Context start(Context context, Attributes attributes);

/** Listener method that is called at the end of a request. */

void end(Context context, Attributes attributes);

Context start(Context context, Attributes requestAttributes);

/** Listener method that is called at the end of a request. */

void end(Context context, Attributes responseAttributes);

…nstrumentation into http-request-metrics

trask · 2021-05-12T16:28:13Z

we have (some) metrics 🎉

Record HTTP server metrics in instrumenter API.

df997ee

anuraaga requested review from trask, jkwatson, iNikem and mateuszrzeszutek April 27, 2021 08:49

Anuraag Agrawal added 2 commits April 27, 2021 17:50

Fix

80e5c44

Response attributes

7d84f0f

anuraaga commented Apr 27, 2021

View reviewed changes

jkwatson reviewed Apr 27, 2021

View reviewed changes

richardstartin reviewed Apr 27, 2021

View reviewed changes

mateuszrzeszutek reviewed Apr 29, 2021

View reviewed changes

Anuraag Agrawal added 2 commits May 7, 2021 13:13

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

afb8122

…nstrumentation into http-request-metrics

Start cleaning

54de4d5

anuraaga commented May 7, 2021

View reviewed changes

Anuraag Agrawal added 4 commits May 7, 2021 13:51

Docs

53f92d9

Test

2e3b779

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

abfa41b

…nstrumentation into http-request-metrics

Finish test

cc39b89

anuraaga marked this pull request as ready for review May 10, 2021 06:25

anuraaga requested review from laurit, pavolloffay and tylerbenson as code owners May 10, 2021 06:25

trask approved these changes May 10, 2021

View reviewed changes

Anuraag Agrawal added 2 commits May 11, 2021 12:53

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

4d1dba5

…nstrumentation into http-request-metrics

Cleanups

46435ca

mateuszrzeszutek reviewed May 11, 2021

View reviewed changes

Anuraag Agrawal added 2 commits May 12, 2021 13:07

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

fc11cd4

…nstrumentation into http-request-metrics

Cleanups

beb7252

mateuszrzeszutek approved these changes May 12, 2021

View reviewed changes

anuraaga merged commit 057d886 into open-telemetry:main May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record HTTP server metrics in instrumenter API. #2877

Record HTTP server metrics in instrumenter API. #2877

anuraaga commented Apr 27, 2021

anuraaga Apr 27, 2021

anuraaga Apr 27, 2021

mateuszrzeszutek Apr 29, 2021

anuraaga May 7, 2021

jkwatson Apr 27, 2021

anuraaga Apr 28, 2021

jkwatson Apr 28, 2021

anuraaga Apr 28, 2021

richardstartin Apr 27, 2021 •

edited

Loading

anuraaga Apr 28, 2021

richardstartin Apr 28, 2021

mateuszrzeszutek Apr 29, 2021

mateuszrzeszutek Apr 29, 2021

mateuszrzeszutek Apr 29, 2021

anuraaga May 7, 2021

mateuszrzeszutek Apr 29, 2021

anuraaga May 7, 2021

anuraaga left a comment

anuraaga May 7, 2021

anuraaga May 7, 2021

anuraaga May 7, 2021

anuraaga commented May 10, 2021

mateuszrzeszutek May 11, 2021

anuraaga May 12, 2021

mateuszrzeszutek May 11, 2021

mateuszrzeszutek May 11, 2021

anuraaga May 12, 2021

mateuszrzeszutek May 11, 2021

trask commented May 12, 2021


		Context start(Context context, Attributes requestAttributes);

		void end(Context context, Attributes responseAttributes);

	public InstrumenterBuilder<REQUEST, RESPONSE> addRequestMetrics(RequestMetrics factory) {
	@UnstableApi
	public InstrumenterBuilder<REQUEST, RESPONSE> addRequestMetrics(RequestMetrics factory) {

	public final class HttpServerMetrics implements RequestListener {
	@UnstableApi
	public final class HttpServerMetrics implements RequestListener {

Record HTTP server metrics in instrumenter API. #2877

Record HTTP server metrics in instrumenter API. #2877

Conversation

anuraaga commented Apr 27, 2021

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

richardstartin Apr 27, 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

anuraaga 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

anuraaga commented May 10, 2021

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

trask commented May 12, 2021

richardstartin Apr 27, 2021 •

edited

Loading