Support the `http.request.method_original` attribute by mateuszrzeszutek · Pull Request #8779 · open-telemetry/opentelemetry-java-instrumentation

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

Implements open-telemetry/semantic-conventions#17

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

    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

      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

+      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

    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.

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

+   * @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)

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

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

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

+              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

+  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 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

Conversation

mateuszrzeszutek commented Jun 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breedx-splk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mateuszrzeszutek commented Jul 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurit Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trask left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

laurit Jul 14, 2023 •

edited

Loading