Restlet instrumentation #3946

anosek-an · 2021-08-25T11:00:45Z

Beginning of Restlet instrumentation - issue #3661.

server instrumentation - creation of server span
works with versions 1.0.x and 1.1.x, 1.2 will be checked in future PRs

instrumentation/restlet/restlet-core-1.1/javaagent/build.gradle.kts

...va/io/opentelemetry/javaagent/instrumentation/restlet/v1_1/RestletInstrumentationModule.java

...brary/src/main/java/io/opentelemetry/instrumentation/restlet/v1_1/RestletExtractAdapter.java

...ary/src/main/java/io/opentelemetry/instrumentation/restlet/v1_1/RestletHttpServerTracer.java

...c/main/groovy/io/opentelemetry/instrumentation/restlet/v1_1/AbstractRestletServerTest.groovy

instrumentation/restlet/restlet-core-1.1/library/src/test/groovy/RestletServerTest.groovy

.../main/java/io/opentelemetry/javaagent/instrumentation/restlet/v1_1/RouteInstrumentation.java

...main/java/io/opentelemetry/javaagent/instrumentation/restlet/v1_1/ServerInstrumentation.java

…dule, other review refinements

instrumentation/restlet/restlet-1.0/javaagent/build.gradle.kts

...t-1.0/library/src/main/java/io/opentelemetry/instrumentation/restlet/v1_0/TracingFilter.java

...c/main/java/io/opentelemetry/instrumentation/restlet/v1_0/RestletNetAttributesExtractor.java

.../main/java/io/opentelemetry/instrumentation/restlet/v1_0/RestletHttpAttributesExtractor.java

...0/library/src/main/java/io/opentelemetry/instrumentation/restlet/v1_0/RestletSingletons.java

...main/java/io/opentelemetry/javaagent/instrumentation/restlet/v1_0/ServerInstrumentation.java

.../main/java/io/opentelemetry/javaagent/instrumentation/restlet/v1_0/RouteInstrumentation.java

mateuszrzeszutek

Looks nice! I left just a few (mostly minor) comments.

...ary/src/main/java/io/opentelemetry/instrumentation/restlet/v1_0/RestletServerSpanNaming.java

...-1.0/library/src/main/java/io/opentelemetry/instrumentation/restlet/v1_0/RestletTracing.java

...ary/src/main/java/io/opentelemetry/instrumentation/restlet/v1_0/RestletServerSpanNaming.java

...-1.0/library/src/main/java/io/opentelemetry/instrumentation/restlet/v1_0/RestletTracing.java

...brary/src/main/java/io/opentelemetry/instrumentation/restlet/v1_0/RestletTracingBuilder.java

...t-1.0/library/src/main/java/io/opentelemetry/instrumentation/restlet/v1_0/TracingFilter.java

mateuszrzeszutek

LGTM 👍
Thanks!

trask · 2021-09-09T02:45:53Z

...main/java/io/opentelemetry/javaagent/instrumentation/restlet/v1_0/ServerInstrumentation.java

+        return;
+      }
+
+      // Restlet suppresses exceptions and sets the throwable in status


trask · 2021-09-09T02:48:41Z

...ibrary/src/main/java/io/opentelemetry/instrumentation/restlet/v1_0/RestletHeadersGetter.java

+  public Iterable<String> keys(Request carrier) {
+    return HeadersAdapter.getHeaders(carrier).getNames();
+  }
+
+  @Override
+  public String get(Request carrier, String key) {
+    return HeadersAdapter.getValue(carrier, key);
+  }


I'd suggest inlining the HeadersAdapter methods and deleting the HeadersAdapter class

sure, I think it's not necessary to keep them separate

trask · 2021-09-09T02:55:11Z

...-1.0/library/src/main/java/io/opentelemetry/instrumentation/restlet/v1_0/RestletTracing.java

+  public Instrumenter<Request, Response> getServerInstrumenter() {
+    return serverInstrumenter;
+  }


maybe just getInstrumenter() if we don't need to distinguish between client and server instrumenter

I'm planning on adding client instrumentation in the future, so I'd prefer to leave the current naming

make sense 👍

trask · 2021-09-09T02:59:01Z

...ry/src/test/groovy/io/opententelemetry/instrumentation/restlet/v1_0/RestletServerTest.groovy

+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+


Suggested change

/*

* Copyright The OpenTelemetry Authors

* SPDX-License-Identifier: Apache-2.0

*/

trask · 2021-09-09T03:02:48Z

...ry/src/test/groovy/io/opententelemetry/instrumentation/restlet/v1_0/RestletServerTest.groovy

+
+  @Override
+  boolean testException() {
+    false //Filter's afterHandle does not execute if exception was thrown in the next restlet


if afterHandle is not executed, will the Scope that was started in beforeHandle be left open (and the Context left bound to the thread)?

after some investigation and looking into Restlet's StatusFilter I found a way to make afterHandle execute even if an exception was thrown, so the Scope will be safely closed in every case now

trask · 2021-09-09T03:03:27Z

...c/main/groovy/io/opentelemetry/instrumentation/restlet/v1_0/AbstractRestletServerTest.groovy

+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS
+
+abstract class AbstractRestletServerTest extends HttpServerTest<Server> {


…hods

mateuszrzeszutek · 2021-09-13T11:58:50Z

...t-1.0/library/src/main/java/io/opentelemetry/instrumentation/restlet/v1_0/TracingFilter.java

+    try {
+      super.doHandle(request, response);
+    } catch (Throwable t) {
+      response.setStatus(new Status(Status.SERVER_ERROR_INTERNAL, t));


You could implement the whole instrumentation inside doHandle to avoid modifying the response object

Without setting status this way, the throwable is not propagated and we get a response with status of 200, not 500. My approach based on StatusFilter.
I can move instrumentation code to doHandle, leaving this line as it is.

I see. How about you move the instrumentation code to doHandle and use the StatusFilter in library instrumentation tests? We generally want an instrumentation to be as transparent as possible - I agree that returning 200 for unhandled exceptions is a bit weird and unintuitive, but as instrumentation authors we should avoid changing this default behavior on our whim. If a user wants to return 500s instead then they're going to use StatusFilter anyway.

Sure, thanks for the explanation!

trask

thx @Enkelian!

anosek-an requested review from anuraaga, iNikem, jkwatson, laurit, mateuszrzeszutek, pavolloffay, trask and tylerbenson as code owners August 25, 2021 11:00

anosek-an mentioned this pull request Aug 25, 2021

REQUEST: New membership for Enkelian open-telemetry/community#817

Closed

6 tasks

mateuszrzeszutek reviewed Aug 27, 2021

View reviewed changes

anosek-an added 3 commits September 6, 2021 11:42

server instrumentation with first tests

30ebf44

migrate to instrumenter API, move TracingFilter to library, rename mo…

3bbbf83

…dule, other review refinements

change name to 1.0 and rebase

3e095a5

anosek-an force-pushed the restlet-server-instrumentation branch from cf73b9c to 3e095a5 Compare September 6, 2021 10:12

mateuszrzeszutek reviewed Sep 7, 2021

View reviewed changes

anosek-an added 2 commits September 8, 2021 10:25

review, add ServerSpanNaming, create RestletTracing

12cc936

codenarc fix

4830b43

mateuszrzeszutek reviewed Sep 8, 2021

View reviewed changes

review

66fe657

mateuszrzeszutek approved these changes Sep 8, 2021

View reviewed changes

trask reviewed Sep 9, 2021

View reviewed changes

fix TracingFilter behaviour on exception, inline HeadersAdapter's met…

dd9481b

…hods

mateuszrzeszutek approved these changes Sep 13, 2021

View reviewed changes

move instrumentation to doHandle, add StatusFilter in library test

23bbf0d

mateuszrzeszutek approved these changes Sep 13, 2021

View reviewed changes

trask approved these changes Sep 13, 2021

View reviewed changes

trask merged commit 473f16f into open-telemetry:main Sep 14, 2021

anosek-an deleted the restlet-server-instrumentation branch November 15, 2021 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restlet instrumentation #3946

Restlet instrumentation #3946

anosek-an commented Aug 25, 2021

mateuszrzeszutek left a comment

mateuszrzeszutek left a comment

trask Sep 9, 2021

trask Sep 9, 2021

anosek-an Sep 13, 2021

trask Sep 9, 2021

anosek-an Sep 13, 2021

trask Sep 13, 2021

trask Sep 9, 2021

trask Sep 9, 2021

anosek-an Sep 13, 2021

trask Sep 9, 2021

mateuszrzeszutek Sep 13, 2021

anosek-an Sep 13, 2021

mateuszrzeszutek Sep 13, 2021

anosek-an Sep 13, 2021

trask left a comment

Restlet instrumentation #3946

Restlet instrumentation #3946

Conversation

anosek-an commented Aug 25, 2021

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

mateuszrzeszutek 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

trask left a comment

Choose a reason for hiding this comment