Support the `http.request.method_original` attribute #8779

mateuszrzeszutek · 2023-06-21T14:20:41Z

Implements open-telemetry/semantic-conventions#17

mateuszrzeszutek · 2023-06-21T14:23:06Z

...va/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractor.java

    this(
        httpAttributesGetter,
        netAttributesGetter,
        capturedRequestHeaders,
        capturedResponseHeaders,
+        knownMethods,
        HttpRouteHolder::getRoute);


The complexity of these constructors is kinda starting to get out of hand; I'm planning to implement Jason's dependency injection comment once I merge all the in-flight PRs.

Because this constructor is called all over the place with the default set, you might (in the short term) also consider doing a constructor overload that defaults the set. I think that applies to the client attr extractor as well.

mateuszrzeszutek · 2023-06-21T14:24:36Z

...brary/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetryBuilder.java

@@ -24,6 +26,7 @@ public final class OkHttpTelemetryBuilder {
      new ArrayList<>();
  private List<String> capturedRequestHeaders = emptyList();
  private List<String> capturedResponseHeaders = emptyList();
+  @Nullable private Set<String> knownMethods = null;


I really dislike these nulls here; but this was the least invasive way of implementing an optionally unset set of methods. I've an idea on how to refactor these, but I'll tackle that in a separate PR.

breedx-splk

Looking good to me, just had a few small ideas. Thanks!

breedx-splk · 2023-06-23T17:18:23Z

...va/io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java

+      if (knownMethods.contains(method)) {
+        internalSet(attributes, HttpAttributes.HTTP_REQUEST_METHOD, method);
+      } else {
+        internalSet(attributes, HttpAttributes.HTTP_REQUEST_METHOD, _OTHER);


static import the names to improve readability?

The HttpAttributes values? I'm in favor of that actually -- but if we're doing that, we should implement it for all attribute keys across the whole package. I think I'd prefer to make it a separate PR, if you don't mind

breedx-splk · 2023-06-23T17:24:41Z

...va/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractor.java

    this(
        httpAttributesGetter,
        netAttributesGetter,
        capturedRequestHeaders,
        capturedResponseHeaders,
+        knownMethods,
        HttpRouteHolder::getRoute);


Because this constructor is called all over the place with the default set, you might (in the short term) also consider doing a constructor overload that defaults the set. I think that applies to the client attr extractor as well.

...ry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorStableSemconvTest.java

...ry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorStableSemconvTest.java

...brary/src/main/kotlin/io/opentelemetry/instrumentation/ktor/v2_0/server/KtorServerTracing.kt

...elemetry/instrumentation/netty/v4/common/internal/server/NettyServerInstrumenterFactory.java

breedx-splk · 2023-06-23T17:49:40Z

...brary/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetryBuilder.java

+   * @param knownMethods A set of recognized HTTP request methods.
+   */
+  @CanIgnoreReturnValue
+  public OkHttpTelemetryBuilder setKnownMethods(Set<String> knownMethods) {


@Nullable on the param?

Collections should never be nullable in the API (see the comment above)

...java/io/opentelemetry/instrumentation/spring/webflux/v5_3/SpringWebfluxTelemetryBuilder.java

...-extension-api/src/main/java/io/opentelemetry/javaagent/bootstrap/internal/CommonConfig.java

mateuszrzeszutek · 2023-07-14T10:02:50Z

@open-telemetry/java-instrumentation-approvers please review this PR 🙏

laurit · 2023-07-14T10:36:45Z

...mentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/HttpConstants.java

+              asList(
+                  "CONNECT", "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT", "TRACE")));
+
+  public static final String _OTHER = "_OTHER";


why the _?

It was chosen because it makes the "other" default value look like it's a kinda special thing (which it is)

If the HTTP request method is not known to instrumentation, it MUST set the http.request.method attribute to _OTHER and, except if reporting a metric, MUST set the exact method received in the request line as value of the http.request.method_original attribute.

If it comes from the spec then so it must be. In my opinion using _OTHER is a mistake, there is no way that this is not going to confuse users. Everybody who sees it will assume that this is a bug/typo.

🤷
I don't really have a strong opinion on the value itself; I do agree that you need to have some sort of indication that it's some other unrecognized value though.

this is probably the most relevant comment why _OTHER was chosen: open-telemetry/semantic-conventions#17 (comment)

I could see backends potentially displaying this metric value with a special tooltip to help ease user confusion

trask · 2023-07-14T17:14:49Z

...y/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/NettyServerTelemetryBuilder.java

+  public NettyServerTelemetryBuilder setKnownMethods(Set<String> knownMethods) {
+    this.knownMethods = knownMethods;
+    return this;
+  }


does it make any sense to have HttpClientTelemetryBuilder interface to help enforce that these implementations are consistent (and maybe we can inherit javadoc)?

(and maybe we can inherit javadoc)?

Hmm, we can put a @see tag I think. I don't think we can actually enforce that though.

trask

I'll open an issue to add common http client/server tests which exercise the non-standard methods (I don't think it's a high priority since it's an edge case anyways)

mateuszrzeszutek requested a review from a team as a code owner June 21, 2023 14:20

mateuszrzeszutek commented Jun 21, 2023

View reviewed changes

breedx-splk reviewed Jun 23, 2023

View reviewed changes

Mateusz Rzeszutek added 2 commits July 14, 2023 11:59

Support the http.request.method_original attribute

9423206

code review comments

4e07ef2

mateuszrzeszutek force-pushed the http.request.method_original branch from b350a13 to 4e07ef2 Compare July 14, 2023 10:02

laurit reviewed Jul 14, 2023

View reviewed changes

laurit approved these changes Jul 14, 2023

View reviewed changes

trask reviewed Jul 14, 2023

View reviewed changes

trask approved these changes Jul 14, 2023

View reviewed changes

@see

0bf9aa8

mateuszrzeszutek enabled auto-merge (squash) July 17, 2023 08:53

mateuszrzeszutek merged commit cc8160c into open-telemetry:main Jul 17, 2023
45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support the `http.request.method_original` attribute #8779

Support the `http.request.method_original` attribute #8779

mateuszrzeszutek commented Jun 21, 2023

mateuszrzeszutek Jun 21, 2023

breedx-splk Jun 23, 2023

mateuszrzeszutek Jun 21, 2023

breedx-splk left a comment

breedx-splk Jun 23, 2023

mateuszrzeszutek Jun 26, 2023

breedx-splk Jun 23, 2023

breedx-splk Jun 23, 2023

mateuszrzeszutek Jun 26, 2023

mateuszrzeszutek commented Jul 14, 2023

laurit Jul 14, 2023

mateuszrzeszutek Jul 14, 2023

laurit Jul 14, 2023 •

edited

mateuszrzeszutek Jul 14, 2023

trask Jul 14, 2023

trask Jul 14, 2023

mateuszrzeszutek Jul 17, 2023

trask left a comment

Support the http.request.method_original attribute #8779

Support the http.request.method_original attribute #8779

Conversation

mateuszrzeszutek commented Jun 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breedx-splk 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

mateuszrzeszutek commented Jul 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurit Jul 14, 2023 • edited

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 left a comment

Choose a reason for hiding this comment

Support the `http.request.method_original` attribute #8779

Support the `http.request.method_original` attribute #8779

laurit Jul 14, 2023 •

edited