search

grpc-ecosystem / grpc-spring

Spring Boot starter module for gRPC framework.
https://grpc-ecosystem.github.io/grpc-spring/
Apache License 2.0
3.52k stars 826 forks source link

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

Open SOOHYUN-LIM opened 1 year ago

SOOHYUN-LIM commented 1 year ago

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.

ST-DDT commented 1 year ago

Can you please add a test for this?

SOOHYUN-LIM commented 1 year ago

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.

ST-DDT commented 1 year ago

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

abhathal commented 11 months ago

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

SOOHYUN-LIM commented 11 months ago

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

ST-DDT commented 11 months ago

Could you please resolve the merge conflict?

SOOHYUN-LIM commented 11 months ago

@ST-DDT I resolved the merge conflict

ST-DDT commented 11 months ago

@ST-DDT I resolved the merge conflict

Thanks, I will get to it shortly.

ST-DDT commented 11 months ago

It looks like there is a compile error.

SOOHYUN-LIM commented 11 months ago

@ST-DDT Sorry, Please build it again

msajawalsial commented 6 months ago

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

SOOHYUN-LIM commented 6 months ago

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?

Benji1109 commented 5 months ago

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

ST-DDT commented 5 months ago

If somebody else approves it.

gale-harrington-upstart commented 3 months ago

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

ST-DDT commented 3 months ago

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