sun.net.www.protocol.http.HttpURLConnection.getOutputStream should tr… #6053

jeanbisutti · 2022-05-17T12:32:20Z

The purpose of this PR is to fix #4597

…ansform GET into POST

instrumentation/http-url-connection/javaagent/src/test/groovy/HttpUrlConnectionTest.groovy

laurit · 2022-05-17T14:26:29Z

...pentelemetry/javaagent/instrumentation/httpurlconnection/SunHttpUrlHttpAttributesGetter.java

+  public String method(HttpURLConnection connection) {
+    String requestMethod = connection.getRequestMethod();
+
+    if ("GET".equals(requestMethod)) {


Did you consider doing this inside HttpUrlHttpAttributesGetter?
If you did it this way because there isn't an obvious way how to tell that getOutputStream was called in HttpUrlHttpAttributesGetter then here's a couple of ideas for you. You could try using HttpUrlState, add a boolean field to it that would signify that it really is a POST. Another way would be to use addContextCustomizer method from InstrumenterBuilder. The idea is to use addContextCustomizer to add a holder object into context where you can store info like is this really a POST. For example see https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/grizzly-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grizzly/GrizzlyErrorHolder.java. As you don't have access to context in HttpUrlHttpAttributesGetter.method you'll need to add a custom AttributesExtractor to your Instrumenter. Just leave HttpUrlHttpAttributesGetter as is and overwrite the request method attribute in your AttributesExtractor.onEnd. If you set the SemanticAttributes.HTTP_METHOD attribute in onEnd then it will work even when the span wasn't started from getOutputStream.

Another idea would be to change the request type from HttpURLConnection to a value class that'd contain both HttpURLConnection and the String methodName

Thank you for your suggestions @laurit and @mateuszrzeszutek. In my last commit, I created a new implementation to not depend on the execution order of the connection's methods.

...o/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionSingletons.java

mateuszrzeszutek · 2022-05-17T14:55:38Z

...pentelemetry/javaagent/instrumentation/httpurlconnection/SunHttpUrlHttpAttributesGetter.java

+  public String method(HttpURLConnection connection) {
+    String requestMethod = connection.getRequestMethod();
+
+    if ("GET".equals(requestMethod)) {


Another idea would be to change the request type from HttpURLConnection to a value class that'd contain both HttpURLConnection and the String methodName

mateuszrzeszutek · 2022-05-17T15:00:32Z

instrumentation/http-url-connection/javaagent/src/test/groovy/HttpUrlConnectionTest.groovy

+
+      def connectionClassName = connection.getClass().getCanonicalName()
+
+      assert "sun.net.www.protocol.http.HttpURLConnection".equals(connectionClassName)


Will that work on non-OpenJDK JVMs? Can you add some assumption that will skip this test if this class is not present?

The tests seem to succeed with the OpenJ9 JVM.

I you want, I could add the following kind of code in case where the sun.net.www.protocol.http.HttpURLConnection class disappears one day from the JDK:

if (!"sun.net.www.protocol.http.HttpURLConnection".equals(connectionClassName)) { return; }

I preferred to add only this kind of code if the problem will appear one day, to try to keep things simple.

Nowadays everybody uses class library from openjdk so I wouldn't worry about this class being missing. Though I wouldn't probably build the instrumentation around sun.net.www.protocol.http.HttpURLConnection but rather instrument getOutpuStream in all subclasses of java.net.HttpURLConnection because for this use case, as far as I can tell, it isn't really important on which class getOutpuStream is called. Only the fact that it was called matters. Handling this in other classes besides sun.net.www.protocol.http.HttpURLConnection could come handy on weblogic where there is a custom url protocol handler (system property java.protocol.handler.pkgs can be used to change url protocol handlers).

Though I wouldn't probably build the instrumentation around sun.net.www.protocol.http.HttpURLConnection but rather instrument getOutpuStream in all subclasses of java.net.HttpURLConnection because for this use case, as far as I can tell, it isn't really important on which class getOutpuStream is called.

As far as I understand, the #4597 issue was created for the sun.net.www.protocol.http.HttpURLConnection class. This issue aims at reflecting the getOutputStream() behavior of the sun.net.www.protocol.http.HttpURLConnection class, extending java.net.HttpURLConnection abstract class. Could we be sure that all classes extending java.net.HttpURLConnection will implement a getOutputStream() method that will transform the GET to POST, as in the sun.net.www.protocol.http.HttpURLConnection class?

As far as I understand, the #4597 issue was created for the sun.net.www.protocol.http.HttpURLConnection class. This issue aims at reflecting the getOutputStream() behavior of the sun.net.www.protocol.http.HttpURLConnection class, extending java.net.HttpURLConnection abstract class. Could we be sure that all classes extending java.net.HttpURLConnection will implement a getOutputStream() method that will transform the GET to POST, as in the sun.net.www.protocol.http.HttpURLConnection class?

This is a good question. I believe we can check the value of getRequestMethod after calling getOutputStream, if it returns POST then it was a POST request even if it returned GET before calling getOutputStream.

This is a good question. I believe we can check the value of getRequestMethod after calling getOutputStream, if it returns POST then it was a POST request even if it returned GET before calling getOutputStream.

I am not sure to have been rather explicit. If I was misunderstood., my question was about the expected behavior, not about implementation.

we can check the value of getRequestMethod after calling getOutputStream, if it returns POST then it was a POST request even if it returned GET before calling getOutputStream.

The getOutputStream() method of sun... class updates the method field. So, the getRequestMethod() should return POST even if the HTTP call was done with a GET method.

In term of expected behavior, do we want all classes extending java.net.HttpURLConnection to have an instrumentation behavior for the getOutputStream() method similar to this of sun.net.www.protocol.http.HttpURLConnection? We can't be sure that all classes extending java.net.HttpURLConnection implement the getOutputStream() method the same way as the sun.net.www.protocol.http.HttpURLConnection does (that is to say, changing GET into POST). As in #4597, it may be disturbing not to have instrumentation aligned with the code behavior?

Some comments:

I committed a new implementation to not depend on the execution order of the connection's methods. Could you please review it?

In term of expected behavior, do we want all classes extending java.net.HttpURLConnection to have an instrumentation behavior for the getOutputStream() method similar to this of sun.net.www.protocol.http.HttpURLConnection? We can't be sure that all classes extending java.net.HttpURLConnection implement the getOutputStream() method the same way as the sun.net.www.protocol.http.HttpURLConnection does (that is to say, changing GET into POST). As in https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/4597, it may be disturbing not to have instrumentation aligned with the code behavior?

The new implementation could be adjusted once the expected behavior is determined.

The new implementation updates the HTTP method attribute but not the span name. Does span name also need to be updated? If yes, please advise for the implementation. The HttpSpanNameExtractor has no access to Context. I have noticed that the Instrumenter class
span name extractor calls the span name extractor and has access to the context. So, a possible implementation would be to add Context parameter to the existing method of the SpanNameExtractor interface. It may require updating the code in many places. Another option would be to allow the possibility to define a SpanNameExtractor and call it around here in Instrumenter class. If something needs to be done with the span name, perhaps it could be another PR.

If you believe it is best to keep the checks for sun.net.www.protocol.http.HttpURLConnection then that is ok.

Nice catch about the span name. I think we should change it as it too. You could use

Span span = Span.fromContext(context); span.udateName("HTTP " + requestMethod);

to update the span name too. You could update the span name at the same place where you change the value for HTTP_METHOD attribute.

Code updated to set the span name, thank you for the trick

...o/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionSingletons.java

Co-authored-by: Mateusz Rzeszutek <[email protected]>

…'s methods

laurit · 2022-05-23T11:08:23Z

...ntelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionInstrumentation.java

@@ -116,6 +116,11 @@ public static void methodExit(
      callDepth.getAndIncrement();
      try {
        scope.close();
+        Class<? extends HttpURLConnection> connectionClass = connection.getClass();
+
+        GetOutputStreamContext getOutputStreamContext =


I'd use the value of connection.getRequestMethod here. If getOutputStream got an exception (maybe setDoOutput(true)) wasn't called then the request method isn't changed and we could still report it as GET.

Very good point! I haved added a condition on the request method with a comment. I propose to leave the condition on the method name of HttpUrlConnection to try to make the code explicit.

laurit · 2022-05-23T11:12:37Z

...ava/io/opentelemetry/javaagent/instrumentation/httpurlconnection/GetOutputStreamContext.java

+    return context.get(KEY);
+  }
+
+  public void set(


In other places that use the same pattern we typically have static methods for setting and getting values.

Code changed

trask

thx @jeanbisutti!

I tend to agree with @laurit thought about implementing this behavior for all subclasses of HttpURLConnection. I would hazzard to guess that weblogic's HttpURLConnection implementation probably had to conform to the sun behavior just to avoid breaking applications which "worked on tomcat" 😄.

But I'm also ok with handling only the sun implementation.

instrumentation/http-url-connection/javaagent/src/test/groovy/HttpUrlConnectionTest.groovy

.../opentelemetry/javaagent/instrumentation/httpurlconnection/HttpMethodAttributeExtractor.java

trask · 2022-05-24T05:03:57Z

...ntelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionInstrumentation.java

+        Class<? extends HttpURLConnection> connectionClass = connection.getClass();
+
+        String requestMethod = connection.getRequestMethod();
+        GetOutputStreamContext.set(
+            httpUrlState.context, connectionClass, methodName, requestMethod);


inlining connectionClass might be nice to get rid of the generic reference above

I would hazzard to guess that weblogic's HttpURLConnection implementation probably had to conform to the sun behavior

Maybe , maybe not. We may discuss this point and create a PR outside this issue? This PR was to fix #4597.

...ava/io/opentelemetry/javaagent/instrumentation/httpurlconnection/GetOutputStreamContext.java

…HttpUrlConnectionTest.groovy Co-authored-by: Trask Stalnaker <[email protected]>

…/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpMethodAttributeExtractor.java Co-authored-by: Trask Stalnaker <[email protected]>

trask · 2022-05-25T17:28:27Z

thx @jeanbisutti!

jeanbisutti · 2022-05-25T17:35:20Z

Thank you very much @laurit, @mateuszrzeszutek and @trask for your help on this first issue!

jeanbisutti requested a review from a team May 17, 2022 12:32

sun.net.www.protocol.http.HttpURLConnection.getOutputStream should tr…

86d20ae

…ansform GET into POST

jeanbisutti force-pushed the 4597-2 branch from f85fa98 to 86d20ae Compare May 17, 2022 13:03

laurit reviewed May 17, 2022

View reviewed changes

mateuszrzeszutek reviewed May 17, 2022

View reviewed changes

jeanbisutti and others added 2 commits May 18, 2022 14:58

Code simplification

c00b3bc

Co-authored-by: Mateusz Rzeszutek <[email protected]>

New implementation to not depend on the execution order of connection…

f560763

…'s methods

jeanbisutti requested review from laurit and mateuszrzeszutek May 23, 2022 09:52

laurit reviewed May 23, 2022

View reviewed changes

jeanbisutti added 3 commits May 23, 2022 14:59

Add condition on request method in case of exception

85bdbfd

Code uniformity

d43e1f3

Update span name

6ef58dc

trask approved these changes May 24, 2022

View reviewed changes

jeanbisutti and others added 5 commits May 24, 2022 10:21

Update instrumentation/http-url-connection/javaagent/src/test/groovy/…

52329ba

…HttpUrlConnectionTest.groovy Co-authored-by: Trask Stalnaker <[email protected]>

Update instrumentation/http-url-connection/javaagent/src/test/groovy/…

27653ab

…HttpUrlConnectionTest.groovy Co-authored-by: Trask Stalnaker <[email protected]>

Update instrumentation/http-url-connection/javaagent/src/main/java/io…

a93fd48

…/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpMethodAttributeExtractor.java Co-authored-by: Trask Stalnaker <[email protected]>

Add volatile

78ad06b

Format code

fd8bee7

laurit approved these changes May 25, 2022

View reviewed changes

trask merged commit e8ee277 into open-telemetry:main May 25, 2022

jeanbisutti mentioned this pull request Jul 12, 2022

REQUEST: New membership for @jeanbisutti open-telemetry/community#1108

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sun.net.www.protocol.http.HttpURLConnection.getOutputStream should tr… #6053

sun.net.www.protocol.http.HttpURLConnection.getOutputStream should tr… #6053

jeanbisutti commented May 17, 2022

laurit May 17, 2022 •

edited

Loading

mateuszrzeszutek May 17, 2022

jeanbisutti May 23, 2022

mateuszrzeszutek May 17, 2022

mateuszrzeszutek May 17, 2022

jeanbisutti May 18, 2022

laurit May 18, 2022

jeanbisutti May 19, 2022

laurit May 19, 2022

jeanbisutti May 19, 2022

jeanbisutti May 23, 2022

laurit May 23, 2022

jeanbisutti May 23, 2022

laurit May 23, 2022

jeanbisutti May 23, 2022

laurit May 23, 2022

jeanbisutti May 23, 2022

trask left a comment

trask May 24, 2022

jeanbisutti May 24, 2022

trask commented May 25, 2022

jeanbisutti commented May 25, 2022 •

edited

Loading


		def connectionClassName = connection.getClass().getCanonicalName()

		assert "sun.net.www.protocol.http.HttpURLConnection".equals(connectionClassName)

sun.net.www.protocol.http.HttpURLConnection.getOutputStream should tr… #6053

sun.net.www.protocol.http.HttpURLConnection.getOutputStream should tr… #6053

Conversation

jeanbisutti commented May 17, 2022

laurit May 17, 2022 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask commented May 25, 2022

jeanbisutti commented May 25, 2022 • edited Loading

laurit May 17, 2022 •

edited

Loading

jeanbisutti commented May 25, 2022 •

edited

Loading