Netflix / dgs-framework

GraphQL for Java with Spring Boot made easy.
https://netflix.github.io/dgs
Apache License 2.0
3.06k stars 295 forks source link

Replace ByteBuddy with Java Proxy #1904

Closed AyushChaubey closed 4 months ago

AyushChaubey commented 4 months ago

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 json files.

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 like reflect-config.json, resource-config.json and 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 DGS using gradle test task and provide the tracing agent parameter with the path where it should generate these metadata json files.

tasks.withType<Test> {
        jvmArgs("-agentlib:native-image-agent=config-output-dir=./src/main/resources/META-INF/native-image")
}

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.

paulbakker commented 4 months ago

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.

AyushChaubey commented 4 months ago

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.

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.

AyushChaubey commented 4 months ago

@AyushChaubey can you double check that you're ok with the changes I made to your branch?

Changes look good @paulbakker. Appreciate your help with this. Thanks šŸ™‡ā€ā™‚ļø.

paulbakker commented 4 months ago

@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.

AyushChaubey commented 4 months ago

@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.