Fix metric disable when using the constructor injection of the Grpc Client #907
Conversation
…pcClientBeanPostProcessor
|
Can you please add a test for this? |
|
I have modified the code to ensure that each dependency injection method works correctly, taking into consideration the Spring lifecycle. Also added test code to verify if metric information registration is done successfully. |
|
Thanks, I will have a look when I have some spare time. |
|
hey @ST-DDT @SOOHYUN-LIM I know this PR might potentially be out of date now, but any update on getting this fix in? |
|
Could you please resolve the merge conflict? |
|
@ST-DDT I resolved the merge conflict |
Thanks, I will get to it shortly. |
|
It looks like there is a compile error. |
|
@ST-DDT Sorry, Please build it again |
There was a problem hiding this comment.
I tested the changes and to make the test pass, all that is needed is moving the initGrpClientConstructorInjections() call, the other changes to the GrpcClientBeanPostProcessor seem to do nothing in the given test.
Could you please explain what the other code's purpose is?
I have a rough guess, that it might be an optimization for the normal field/method injection, but it calls the entire field/method processing for every field and method once again.
With these changes it might be possible to avoid requiring the @PostConstruct workaround entirely. Should we move that refactoring to a future/different PR?
| @@ -149,8 +152,7 @@ List<GrpcChannelConfigurer> defaultChannelConfigurers() { | |||
| "io.grpc.netty.shaded.io.grpc.netty.NettyChannelBuilder"}) | |||
| @Bean | |||
| @Lazy | |||
| GrpcChannelFactory shadedNettyGrpcChannelFactory( | |||
| final GrpcChannelsProperties properties, | |||
| GrpcChannelFactory shadedNettyGrpcChannelFactory(final GrpcChannelsProperties properties, | |||
There was a problem hiding this comment.
Please revert these unrelated formatting changes.
|
Is there an estimated timeline for merging this pull request? In addition to metrics, it is also affecting tracing. |
SOOHYUN-LIM
force-pushed
the
fix-GrpcClientBeanPostProcessor
branch
from
b07e9a2
to
3f3bd29
Compare
|
Sorry for the delay. During the initialization of LoadTimeWeaverAware (LTW), I performed the initialization of the grpc client BEAN. This ensures that at that point of initialization, any potential issues arising from other associated beans being registered in unintended orders are prevented. It has been confirmed that initialization occurs only once. Could you please provide an explanation once again? |
|
@ST-DDT @SOOHYUN-LIM |
|
If somebody else approves it. |
|
@ST-DDT @SOOHYUN-LIM bumping the question on when this PR might be merged |
|
@yidongnan Can you please name a person that can review/approve PRs for this repository going forward? |
�Issue: https://github.com/yidongnan/grpc-spring-boot-starter/issues/859
According to the order of registering Spring beans, there is an issue with the constructor injection of the Grpc Client. When creating the GrpcClientBeanPostProcessor bean, the constructor injection of the "Grpc Client" is performed by the @PostConstruct. During this process, when creating the GlobalGrpcClientInterceptor and its associated beans (MetricsBeanPostProcessor, Prometheus, etc.), the initialized "BeanPostProcessor" is not yet registered (nonOrdered), causing metrics to not be properly registered.
Therefore, it is necessary to change the order of bean registration by using InternalPostProcessors, similar to the AutowiredAnnotationBeanPostProcessor in Spring and the PersistenceAnnotationBeanPostProcessor in JPA. The task has been carried out along with the same validation as Spring's dependency injection, and it allows for identifying more specific errors.