apache httpclient bytebuddy #161
Conversation
realark
force-pushed
the
ark/apache_http_client_bytebuddy
branch
from
9338c9a to
397864d
Compare
There was a problem hiding this comment.
This is actually why I included ratpack test utilities. Take a look at DDApiTest for examples on how I'm using it. I think it will work under junit+java style tests as well.
There was a problem hiding this comment.
Nice! I'll move my tests over so all tests use the same embedded server.
There was a problem hiding this comment.
This class seems a bit complicated. I'm wondering if there is a better way to trace this.
There was a problem hiding this comment.
I agree; the instrumentation approach should be revisited (I don't know for sure if there's a better way, but I share your feeling on the matter). However, doing that would delay moving away from byteman.
This is the same instrumentation we were using before (in byteman). I removed the opentracing-contrib dependency and moved the helper into our project.
I think it makes sense to separate the bytebuddy migration from the instrumentation approach. Here's what I propose:
- Keep this PR confined to porting the instrumentation over to bytebuddy
- Provided this doesn't increase the project's technical debt.
- Add an issue to the backlog for investigating the overall instrumentation approach and prioritize against other needs.
There was a problem hiding this comment.
Ah, I didn't realize that was something that was already complicated. I thought that was part of your "new approach". Why did you need to bring it into our repo again?
There was a problem hiding this comment.
When we patch HttpClientBuilder (at classload time) we inject our helper classes. TracingHttpClientBuilder extends HttpClientBuilder, which causes a load of HttpClientBuilder while loading HttpClientBuilder, which causes a LinkageError in the user's app.
The only purpose of TracingHttpClientBuilder is to inject TracingClientExec, so I moved TracingClientExec into our project and changed the injection strategy. Since TracingHttpClientBuilder is no longer a helper the circularity error goes away.
realark
force-pushed
the
ark/apache_http_client_bytebuddy
branch
from
397864d to
c7e46f9
Compare
realark
force-pushed
the
ark/apache_http_client_bytebuddy
branch
from
c7e46f9 to
6fa3030
Compare
realark
force-pushed
the
ark/apache_http_client_bytebuddy
branch
from
550a5ef to
cbd1d3c
Compare
Forcing guava to version 18 prevented running the embedded ratpack http server. Using codesource for the shadow test has the same effect and doesn't require forcing a version of guava.
tylerbenson
left a comment
tylerbenson
left a comment
There was a problem hiding this comment.
Looks great. Nice work.
The dependency on opentracing-apache-contrib has also been removed. Using theses contribs as helper classes was causing LinkageErrors when instrumenting with bytebuddy (details here: https://trello.com/c/96EY75FW/312-instrumentation-may-create-linkage-errors-which-crash-the-users-app).
Tested with client versions 4.3-4.5.3 (latest).