jetty-httpclient-12: send method must pass along implemented response listeners #12326
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stevenschlansker
force-pushed
the
jetty-httpclient-12-listener-interfaces
branch
5 times, most recently
from
f796e4c to
657d6e4
Compare
laurit
reviewed
Sep 25, 2024
...rc/main/java/io/opentelemetry/instrumentation/jetty/httpclient/v12_0/TracingHttpRequest.java
Outdated
Show resolved
Hide resolved
stevenschlansker
force-pushed
the
jetty-httpclient-12-listener-interfaces
branch
3 times, most recently
from
84c0a4f to
24bb28e
Compare
|
@stevenschlansker give it a |
stevenschlansker
force-pushed
the
jetty-httpclient-12-listener-interfaces
branch
2 times, most recently
from
af5b622 to
e8901ca
Compare
|
could you also add a test |
I did look into this, but the current testing structure is quite rigid with stacked superclasses and I am not sure exactly how to extend it since the tests are expected to apply to many different libraries. I'll give it a go... |
… listeners The super HttpRequest.send call says: > The listener passed to this method may implement not only Response.CompleteListener > but also other response listener interfaces, and all the events implemented will be notified. The current implementation passes through a lambda that implements CompleteListener only, and does not delegate-in-scope to any other callback methods you might provide
stevenschlansker
force-pushed
the
jetty-httpclient-12-listener-interfaces
branch
from
e8901ca to
d8a201a
Compare
|
Ok, I tried to add a new test case that checks that listeners are run. |
|
@stevenschlansker I slightly modified the test to also verify that context is propagated to the listener |
laurit
approved these changes
Oct 1, 2024
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
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.
The super HttpRequest.send call says:
The current implementation passes through a lambda that implements CompleteListener only, and does not delegate-in-scope to any other callback methods you might provide