grpc-spring icon indicating copy to clipboard operation
grpc-spring copied to clipboard

Published 20 hours ago • verified publisher icon grpc-ecosystem

Fix metric disable when using the constructor injection of the Grpc Client

Open SOOHYUN-LIM opened this issue 1 year ago • 16 comments

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.

SOOHYUN-LIM avatar Jun 12 '23 05:06 SOOHYUN-LIM

Can you please add a test for this?

ST-DDT avatar Jun 12 '23 06:06 ST-DDT

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.

SOOHYUN-LIM avatar Jun 16 '23 09:06 SOOHYUN-LIM

Thanks, I will have a look when I have some spare time.

ST-DDT avatar Jun 19 '23 18:06 ST-DDT

hey @ST-DDT @SOOHYUN-LIM I know this PR might potentially be out of date now, but any update on getting this fix in?

abhathal avatar Nov 17 '23 02:11 abhathal

@abhathal No, I haven't received any feedback yet. @ST-DDT When can i expect confirmation?

SOOHYUN-LIM avatar Nov 17 '23 07:11 SOOHYUN-LIM

Could you please resolve the merge conflict?

ST-DDT avatar Nov 17 '23 09:11 ST-DDT

@ST-DDT I resolved the merge conflict

SOOHYUN-LIM avatar Nov 21 '23 04:11 SOOHYUN-LIM

@ST-DDT I resolved the merge conflict

Thanks, I will get to it shortly.

ST-DDT avatar Nov 22 '23 07:11 ST-DDT

It looks like there is a compile error.

ST-DDT avatar Nov 22 '23 07:11 ST-DDT

@ST-DDT Sorry, Please build it again

SOOHYUN-LIM avatar Nov 22 '23 08:11 SOOHYUN-LIM

Is there an estimated timeline for merging this pull request? In addition to metrics, it is also affecting tracing.

msajawalsial avatar Apr 21 '24 23:04 msajawalsial

Sorry for the delay. @ST-DDT As you mentioned, relocating initGrpClientConstructorInjections() would resolve the issue. However, given the potential for bugs due to the order of bean registrations, I proceeded with improvement efforts.

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?

SOOHYUN-LIM avatar Apr 23 '24 06:04 SOOHYUN-LIM

@ST-DDT @SOOHYUN-LIM Is there an estimated timeline for merging this pull request?

Benji1109 avatar May 16 '24 12:05 Benji1109

If somebody else approves it.

ST-DDT avatar May 17 '24 08:05 ST-DDT

@ST-DDT @SOOHYUN-LIM bumping the question on when this PR might be merged

gale-harrington-upstart avatar Jul 24 '24 19:07 gale-harrington-upstart

@yidongnan Can you please name a person that can review/approve PRs for this repository going forward?

ST-DDT avatar Jul 24 '24 22:07 ST-DDT