Replace ByteBuddy with Java Proxy #1904
Conversation
AyushChaubey
changed the title
...om/netflix/graphql/dgs/metrics/micrometer/dataloader/DgsDataLoaderInstrumentationProvider.kt
Show resolved
Hide resolved
|
Sorry for the slow reply @AyushChaubey, I was traveling and completely missed the PR. This looks great! I've tested it and it seems to work fine. I also pushed some more changes to your branch, because I realized that we don't need a separate handler now for each type of dataloader, since it's using reflection now. |
| import java.util.concurrent.CompletionStage | ||
|
|
||
| internal class BatchLoaderWithContextInterceptor( | ||
| private val batchLoader: BatchLoaderWithContext<*, *>, | ||
| private val batchLoaderWithContext: Any, |
There was a problem hiding this comment.
Because it's now using reflection the specific interface doesn't actually matter anymore, and we can get away with a single InvocationHandler interceptor that handles all four types of batch loaders.
| @@ -1,7 +1,6 @@ | |||
| dependencies { | |||
| api(project(":graphql-dgs")) | |||
|
|
|||
| implementation("net.bytebuddy:byte-buddy") | |||
There was a problem hiding this comment.
@AyushChaubey can you double check that you're ok with the changes I made to your branch?
No problem @paulbakker. Thanks for looking into the PR. It makes sense, we don't need separate handler for different types of dataloaders. Thanks for pushing the changes. |
Changes look good @paulbakker. Appreciate your help with this. Thanks 🙇♂️. |
|
@AyushChaubey, this is just a heads-up that we got a bit distracted by some urgent issues that needed attention. We were planning to do a bit more testing before releasing this since it's a pretty small but important change. Will be merged and released soon, sorry for the delay. |
No problem at all @paulbakker. Thanks for the update :). I see the PR is merged. Appreciate your time and effort for taking it through. |
Pull Request type
Changes in this PR
Please find the details related to this PR in this discussion
Describe the new behavior from this PR, and why it's needed
ByteBuddy seems to be causing issue when the native image is run. One of the solution to overcome this issue is to use Java Proxy.
Note:
I ran some performance tests from my side on a service. It would be nice if you also have some performance tests that covers this flow.
Instructions on creating a native image:
For a library to support GraalVM, it needs to provide the reachability metadata. GraalVM needs to know about the dynamically-accessed elements(Reflection/Resource loading etc.) at the time of build. This information is provided by the libraries in the form of
jsonfiles.There is this repository, where community contributes this metadata information for different libraries. One example is these metadata json files for
graphql-java. You will find files likereflect-config.json,resource-config.jsonand others.There is a way to generate these metadata files which is using the GraalVM tracing agent. So what we do is, we run the tests in our service that uses
DGSusing gradletesttask and provide the tracing agent parameter with the path where it should generate these metadata json files.By doing this, those metadata json files are generated at this path
src/main/resources/META-INF/native-image. Next when we build the native image, it uses that information and creates a native image that works perfectly.Luckily, other than
DGS micrometer, we didn't face any issue with the native image in terms of the implementation in DGS.We are using Spring Boot, which works with GraalVM to generate the metadata information but DGS would also need to provide this information to reduce the complete dependency on the tracing agent. I guess we would still need to use the tracing agent to cover all the cases which also means writing tests that cover all the scenarios.