Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sun.net.www.protocol.http.HttpURLConnection.getOutputStream should tr… #6053
sun.net.www.protocol.http.HttpURLConnection.getOutputStream should tr… #6053
Changes from all commits
86d20ae
c00b3bc
f560763
85bdbfd
d43e1f3
6ef58dc
52329ba
27653ab
a93fd48
78ad06b
fd8bee7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inlining
connectionClass
might be nice to get rid of the generic reference aboveThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will that work on non-OpenJDK JVMs? Can you add some assumption that will skip this test if this class is not present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:I preferred to add only this kind of code if the problem will appear one day, to try to keep things simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 instrumentgetOutpuStream
in all subclasses ofjava.net.HttpURLConnection
because for this use case, as far as I can tell, it isn't really important on which classgetOutpuStream
is called. Only the fact that it was called matters. Handling this in other classes besidessun.net.www.protocol.http.HttpURLConnection
could come handy on weblogic where there is a custom url protocol handler (system propertyjava.protocol.handler.pkgs
can be used to change url protocol handlers).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, the #4597 issue was created for the
sun.net.www.protocol.http.HttpURLConnection
class. This issue aims at reflecting thegetOutputStream()
behavior of thesun.net.www.protocol.http.HttpURLConnection
class, extendingjava.net.HttpURLConnection
abstract class. Could we be sure that all classes extendingjava.net.HttpURLConnection
will implement agetOutputStream()
method that will transform the GET to POST, as in thesun.net.www.protocol.http.HttpURLConnection
class?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question. I believe we can check the value of
getRequestMethod
after callinggetOutputStream
, if it returnsPOST
then it was aPOST
request even if it returnedGET
before callinggetOutputStream
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure to have been rather explicit. If I was misunderstood., my question was about the expected behavior, not about implementation.
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 thegetOutputStream()
method similar to this ofsun.net.www.protocol.http.HttpURLConnection
? We can't be sure that all classes extendingjava.net.HttpURLConnection
implement thegetOutputStream()
method the same way as thesun.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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toContext
. I have noticed that theInstrumenter
classspan 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 aSpanNameExtractor
and call it around here in Instrumenter class. If something needs to be done with the span name, perhaps it could be another PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sun.net.www.protocol.http.HttpURLConnection
then that is ok.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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code updated to set the span name, thank you for the trick