Put `http.route` attribute onto `http.server.duration` on Play framework request processing #7801

moznion · 2023-02-11T01:42:50Z

Basically, akka-http instrumenter has the responsibility to instrument the http.server.duration for the Play framework application, but the current implementation has not marked the http.route attribute.
ref:

opentelemetry-java-instrumentation/instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/AkkaHttpServerAttributesGetter.java

Line 59 in 8e8161c

return null;

Actually, it's hard to record that attribute by only the akka-http layer because that library's request object doesn't hold the route information, e.g. placeholder.

So this patch delegates that job to the play-mvc instrumenter and when that has been able to get the route info, the instrumenter puts http.route attribute onto http.server.duration.

For example, when the routes configuration of the Play is like the following:

GET  /foo/:bar  controllers.HomeController.doSomething(bar: String)

and when it tries to access that API, then OTEL instruments like so:

http_server_duration_count{otel_scope_name="io.opentelemetry.akka-http-10.0",otel_scope_version="1.23.0-alpha-SNAPSHOT",http_flavor="1.1",http_method="GET",http_route="/foo/$bar<[^/]+>",http_scheme="http",http_status_code="200",net_host_name="localhost",net_host_port="9000"} 1.0 1676078079798
http_server_duration_sum{otel_scope_name="io.opentelemetry.akka-http-10.0",otel_scope_version="1.23.0-alpha-SNAPSHOT",http_flavor="1.1",http_method="GET",http_route="/foo/$bar<[^/]+>",http_scheme="http",http_status_code="200",net_host_name="localhost",net_host_port="9000"} 12183.558843 1676078079798
http_server_duration_bucket{otel_scope_name="io.opentelemetry.akka-http-10.0",otel_scope_version="1.23.0-alpha-SNAPSHOT",http_flavor="1.1",http_method="GET",http_route="/foo/$bar<[^/]+>",http_scheme="http",http_status_code="200",net_host_name="localhost",net_host_port="9000",le="0.0"} 0.0 1676078079798
...
http_server_duration_bucket{otel_scope_name="io.opentelemetry.akka-http-10.0",otel_scope_version="1.23.0-alpha-SNAPSHOT",http_flavor="1.1",http_method="GET",http_route="/foo/$bar<[^/]+>",http_scheme="http",http_status_code="200",net_host_name="localhost",net_host_port="9000",le="+Inf"} 1.0 1676078079798

Rel: #1415

…ork request processing Basically, `akka-http` instrumenter has the responsibility to instrument the `http_server_duration`, but the current implementation has not marked the `http_route` attribute. ref: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/8e8161cb2e5b27d78b8afb7872df17cfa25a2f74/instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/AkkaHttpServerAttributesGetter.java#L59 Actually, it's hard to record that attribute by only the akka-http layer because that library's request object doesn't hold the route information, e.g. placeholder. So this patch delegates that job to the `play` instrumenter and when that has been able to get the route info, the instrumenter puts `http_route` attribute onto `http_server_duration`. For example, when the routes configuration of the Play is like the following: ``` GET /foo/:bar controllers.HomeController.doSomething(bar: String) ``` and when it tries to access that API, then OTEL instruments like so: ```prometheus http_server_duration_count{otel_scope_name="io.opentelemetry.akka-http-10.0",otel_scope_version="1.23.0-alpha-SNAPSHOT",http_flavor="1.1",http_method="GET",http_route="/foo/$bar<[^/]+>",http_scheme="http",http_status_code="200",net_host_name="localhost",net_host_port="9000"} 1.0 1676078079798 http_server_duration_sum{otel_scope_name="io.opentelemetry.akka-http-10.0",otel_scope_version="1.23.0-alpha-SNAPSHOT",http_flavor="1.1",http_method="GET",http_route="/foo/$bar<[^/]+>",http_scheme="http",http_status_code="200",net_host_name="localhost",net_host_port="9000"} 12183.558843 1676078079798 http_server_duration_bucket{otel_scope_name="io.opentelemetry.akka-http-10.0",otel_scope_version="1.23.0-alpha-SNAPSHOT",http_flavor="1.1",http_method="GET",http_route="/foo/$bar<[^/]+>",http_scheme="http",http_status_code="200",net_host_name="localhost",net_host_port="9000",le="0.0"} 0.0 1676078079798 ... http_server_duration_bucket{otel_scope_name="io.opentelemetry.akka-http-10.0",otel_scope_version="1.23.0-alpha-SNAPSHOT",http_flavor="1.1",http_method="GET",http_route="/foo/$bar<[^/]+>",http_scheme="http",http_status_code="200",net_host_name="localhost",net_host_port="9000",le="+Inf"} 1.0 1676078079798 ``` Rel: open-telemetry#1415 Signed-off-by: moznion <[email protected]>

linux-foundation-easycla · 2023-02-11T01:42:55Z

The committers listed above are authorized under a signed CLA.

✅ login: moznion / name: moznion (451c07b)

…bute Signed-off-by: moznion <[email protected]>

trask

thx @moznion!

can you see if you can add a test that shows http.route now getting collected on the spans?

moznion · 2023-02-12T04:29:39Z

@trask sure, I'll add some test cases for this.
I actually wanted to confirm whether this patch's way is acceptable to solve the issue by contributors's review at first. I appreciate your review and comment.

…in a server span with Play framework Signed-off-by: moznion <[email protected]>

moznion · 2023-02-13T08:25:24Z

@trask I added the test case for this patch to the smoke test @ 5151da5.

Essentially, this test case should be in the individual instrumentation subproject (i.e. instrumentation/play/play-mvc/play-mvc-2.6/javaagent/src/test) but it doesn't because the current test code in the subproject uses "custom routing" instead of the actual Play application, so it doesn't inject the HANDLER_DEF value for the request object (see also: playframework/playframework#7829).
Due to this problem, it's hard to do test in the subproject (actually, I'm not sure how to make an application for testing purposes without polution for that subproject. Probably it needs to locate that under the test/ directory but I have no idea how can I do that), thus this diverts the smoke test which uses the actual Play app to check the attribute injection.

Signed-off-by: moznion <[email protected]>

mateuszrzeszutek

Thanks @moznion !

mateuszrzeszutek · 2023-02-13T10:17:55Z

...ent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_6/Play26Singletons.java

    // set the span name on the upstream akka/netty span
    Span serverSpan = LocalRootSpan.fromContextOrNull(context);


Can you remove the code that handles the serverSpan below? The HttpRouteHolder does that already

Thank you for pointing out it. I removed them at ff97ca1.

mateuszrzeszutek · 2023-02-13T10:18:07Z

...ent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_6/Play26Singletons.java

@@ -66,6 +73,10 @@ public static void updateSpanNames(Context context, Request<?> request) {
    }
  }

+  private static void updateHttpRoute(Context context, String route) {


Nit: this method is so simple that it could be inlined.

Yeah, actually this was an intentional thing to try making the aim of the procedure obvious by the method name, but as you mentioned this has already been enough self-descriptive and ff97ca1 removed extra lines, so I changed them to be inlined.

mateuszrzeszutek · 2023-02-13T10:20:55Z

smoke-tests/src/test/groovy/io/opentelemetry/smoketest/PlaySmokeTest.groovy

+    def httpRouteAttributes = extractRouteAttributesFrom(traces)
+    httpRouteAttributes.size() == 1
+    httpRouteAttributes.get(0).value.stringValue == "/welcome"


Nit: you could use TraceInspector and countFilteredAttributes instead.

That's good to know! I simplified that at 5be95b8.

laurit · 2023-02-13T14:13:46Z

...ent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_6/Play26Singletons.java

@@ -52,12 +54,17 @@ public static Instrumenter<Void, Void> instrumenter() {
    return INSTRUMENTER;
  }

-  public static void updateSpanNames(Context context, Request<?> request) {
+  public static void updateContext(Context context, Request<?> request) {


would it be possible to do the same changes in play 2.4 instrumentation?

Good point. I applied the same change for Play 2.4 MVC at 6109ae3.

rel: - open-telemetry#7801 (comment) - open-telemetry#7801 (comment) Signed-off-by: moznion <[email protected]>

ref: https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/7801/files#r1104257504 Signed-off-by: moznion <[email protected]>

rel: open-telemetry#7801 (comment) Signed-off-by: moznion <[email protected]>

Signed-off-by: moznion <[email protected]>

trask · 2023-02-13T22:34:22Z

smoke-tests/src/test/groovy/io/opentelemetry/smoketest/PlaySmokeTest.groovy

@@ -37,6 +38,8 @@ class PlaySmokeTest extends SmokeTest {
    //One internal, one SERVER
    countSpansByName(traces, '/welcome') == 2

+    new TraceInspector(traces).countFilteredAttributes(SemanticAttributes.HTTP_ROUTE.key, "/welcome") == 1


❤️ this is good enough for me, thx!

mateuszrzeszutek

Thanks @moznion !

resolves #7813 caused by conflict between #7730 and #7801

moznion requested a review from a team February 11, 2023 01:42

Leave a note for Play auto instrumentation about http.route attri…

dbc69e4

…bute Signed-off-by: moznion <[email protected]>

github-actions bot requested a review from theletterf February 11, 2023 02:45

trask reviewed Feb 11, 2023

View reviewed changes

Add a smoke test case to check whether the http.route attribute is …

5151da5

…in a server span with Play framework Signed-off-by: moznion <[email protected]>

moznion requested review from trask and removed request for theletterf February 13, 2023 08:25

Fix a violation which is pointed out by :smoke-tests:codenarcTest

871ce5f

Signed-off-by: moznion <[email protected]>

moznion changed the title ~~Put http_route attribute onto http_server_duration on Play framework request processing~~ Put http.route attribute onto http.server.duration on Play framework request processing Feb 13, 2023

github-actions bot requested a review from theletterf February 13, 2023 08:48

mateuszrzeszutek reviewed Feb 13, 2023

View reviewed changes

laurit reviewed Feb 13, 2023

View reviewed changes

moznion added 4 commits February 13, 2023 11:41

Simplify the span updating for Play2.6 MVC

ff97ca1

rel: - open-telemetry#7801 (comment) - open-telemetry#7801 (comment) Signed-off-by: moznion <[email protected]>

Simplify a smoke test case for Play framework

5be95b8

ref: https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/7801/files#r1104257504 Signed-off-by: moznion <[email protected]>

Support http.route attribute in http.server.duration on Play 2.4 MVC

6109ae3

rel: open-telemetry#7801 (comment) Signed-off-by: moznion <[email protected]>

Tidy up the method names for consistency

d52b750

Signed-off-by: moznion <[email protected]>

moznion requested review from mateuszrzeszutek and laurit and removed request for trask, theletterf, mateuszrzeszutek and laurit February 13, 2023 21:23

trask approved these changes Feb 13, 2023

View reviewed changes

trask added this to the v1.23.0 milestone Feb 13, 2023

mateuszrzeszutek approved these changes Feb 14, 2023

View reviewed changes

laurit approved these changes Feb 14, 2023

View reviewed changes

laurit merged commit 7e8d76a into open-telemetry:main Feb 14, 2023

moznion deleted the add_http_route_attribute_on_play branch February 14, 2023 18:01

trask mentioned this pull request Feb 14, 2023

Fix play smoke test #7819

Merged

trask added a commit that referenced this pull request Feb 14, 2023

Fix play smoke test (#7819)

62267fd

resolves #7813 caused by conflict between #7730 and #7801

moznion mentioned this pull request Feb 17, 2023

Play instrumentation tests are not capturing route #1415

Closed

trask mentioned this pull request Feb 23, 2023

fix: get play route from headers instance #7134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put `http.route` attribute onto `http.server.duration` on Play framework request processing #7801

Put `http.route` attribute onto `http.server.duration` on Play framework request processing #7801

moznion commented Feb 11, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 11, 2023 •

edited

Loading

trask left a comment

moznion commented Feb 12, 2023 •

edited

Loading

moznion commented Feb 13, 2023 •

edited

Loading

mateuszrzeszutek left a comment

mateuszrzeszutek Feb 13, 2023

moznion Feb 13, 2023

mateuszrzeszutek Feb 13, 2023

moznion Feb 13, 2023

mateuszrzeszutek Feb 13, 2023

moznion Feb 13, 2023

laurit Feb 13, 2023

moznion Feb 13, 2023

trask Feb 13, 2023

mateuszrzeszutek left a comment

		// set the span name on the upstream akka/netty span
		Span serverSpan = LocalRootSpan.fromContextOrNull(context);

Put http.route attribute onto http.server.duration on Play framework request processing #7801

Put http.route attribute onto http.server.duration on Play framework request processing #7801

Conversation

moznion commented Feb 11, 2023 • edited Loading

linux-foundation-easycla bot commented Feb 11, 2023 • edited Loading

trask left a comment

Choose a reason for hiding this comment

moznion commented Feb 12, 2023 • edited Loading

moznion commented Feb 13, 2023 • edited Loading

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

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

Put `http.route` attribute onto `http.server.duration` on Play framework request processing #7801

Put `http.route` attribute onto `http.server.duration` on Play framework request processing #7801

moznion commented Feb 11, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 11, 2023 •

edited

Loading

moznion commented Feb 12, 2023 •

edited

Loading

moznion commented Feb 13, 2023 •

edited

Loading