Skip to content
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

7.1.0 introduces Spring Boot Micrometer metrics errors #1969

Open
roookeee opened this issue May 2, 2024 · 2 comments
Open

7.1.0 introduces Spring Boot Micrometer metrics errors #1969

roookeee opened this issue May 2, 2024 · 2 comments
Labels
type: bug Something isn't working

Comments

@roookeee
Copy link
Contributor

roookeee commented May 2, 2024

Library Version
7.1.0

Describe the bug
This PR moved the request execution away from netty to the default dispatcher. This causes issues in Micrometer (which we use for metrics) as its not thread-safe and multiple threads provide metrics (the netty thread provides http metrics, DefaultDispatcher based coroutines provide database metrics), see this issue.

We ran into this issue in the past as we did the same thing as the aforementioned graphql-kotlin PR. We had to move away from such a change and saw no performance degradation but graphql-kotlin >= 7.1.0 gives us no way to circumvent this issue now.

To Reproduce

  • Create a spring boot service with micro meter to track your metrics
  • Add graphql-kotlin and add some sample endpoints which use suspend (don't know if suspend is needed)
  • Introduce a lot of load
  • See strange ArrayIndexOutOfBoundsException (see linked micrometer issue)

Expected behavior
Either revert the aforementioned pull request or give us the ability to disable this new feature of moving the request execution to the DefaultDispatcher.

@roookeee roookeee added the type: bug Something isn't working label May 2, 2024
@roookeee roookeee changed the title 7.1 7.1.0 introduces Spring Boot Micrometer metrics errors May 2, 2024
@samuelAndalon
Copy link
Contributor

The spring webflux reference highlights that reactor-http-epoll threads should only be used for handling requests including deserialization, it is very important to not include blocking operations or intensive CPU operations in those thread, thus, moving to a different thread is ideal.

Still, if you need to keep execution there because of an implicit contract with metrics, then you could extend the GraphQLServer class, contributions are always welcomed, so if you want to add an option to make this configurable we will be happy to review it, just keep executing the operation in the default dispatchers coroutine scope.

@roookeee
Copy link
Contributor Author

roookeee commented May 14, 2024

We found another way to add our metric tags without creating the outlined issue. The update to 7.1.x still degrades our services performance by ~10x so we will provide a PR to make this configurable:

7.0.x
image

7.1.x
image

We are fairly certain that this difference is caused by swapping to another thread pool that was introduced in 7.1.x but will get back to you once we know for sure.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Development

No branches or pull requests

2 participants