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

[Task] Refector Metric export #14015

Open
songxiaosheng opened this issue Mar 31, 2024 · 7 comments
Open

[Task] Refector Metric export #14015

songxiaosheng opened this issue Mar 31, 2024 · 7 comments
Labels
component/sdk Related with apache/dubbo help wanted Everything needs help from contributors type/enhancement Everything related with code enhancement or performance

Comments

@songxiaosheng
Copy link
Member

songxiaosheng commented Mar 31, 2024

The implementation of the indicator export part is not abstract enough and cannot be flexibly adapted to other scenarios. You can refer to the implementation of springboot.

@songxiaosheng songxiaosheng added the help wanted Everything needs help from contributors label Mar 31, 2024
@AlbumenJ AlbumenJ added type/enhancement Everything related with code enhancement or performance component/sdk Related with apache/dubbo labels Apr 2, 2024
@yuluo-yx
Copy link
Contributor

@songxiaosheng Can you provide more information? tks.

In spring boot, where should I refer to? this?

@songxiaosheng
Copy link
Member Author

@songxiaosheng Can you provide more information? tks.

In spring boot, where should I refer to? this?

such as this otel PR : https://github.com/apache/dubbo/pull/14067/files
Do you have any ideas about refactoring this piece of code

@yuluo-yx
Copy link
Contributor

@songxiaosheng Can you provide more information? tks.
In spring boot, where should I refer to? this?

such as this otel PR : https://github.com/apache/dubbo/pull/14067/files Do you have any ideas about refactoring this piece of code

spring boot is implemented with the @autoConfiguration annotation. does dubbo have an auto-injection mechanism for this?

@walklown
Copy link

MetricsExport is used to obtain metric information, while MetricsReporter is similar to Register in micrometer. First of all, which part of the current Issue is aimed at optimizing?

And then, I have some optimization suggestions for MetricsExport and MetricsCollector, such as:

  1. Disassemble CombMetricsCollector. ApplicationMetricsCollector, ServiceMetricsCollector, MethodMetricsCollector The binding of the three methods to MetricsKey can exist in Function mode. Now all Collectors must inherit CombMetricsCollector, which is strange.
  2. Split MetricsCollector and MetricsListener. They are different objects.
  3. Consider adjusting the implementation classes of BaseStatComposite and each MetricsExport. These implementation classes are now only related to indicator classification, which is very unfavorable for expansion.

@walklown
Copy link

MetricsExport is used to obtain metric information, while MetricsReporter is similar to Register in micrometer. First of all, which part of the current Issue is aimed at optimizing?

And then, I have some optimization suggestions for MetricsExport and MetricsCollector, such as:

  1. Disassemble CombMetricsCollector. ApplicationMetricsCollector, ServiceMetricsCollector, MethodMetricsCollector The binding of the three methods to MetricsKey can exist in Function mode. Now all Collectors must inherit CombMetricsCollector, which is strange.
  2. Split MetricsCollector and MetricsListener. They are different objects.
  3. Consider adjusting the implementation classes of BaseStatComposite and each MetricsExport. These implementation classes are now only related to indicator classification, which is very unfavorable for expansion.

My suggestions will be implemented in #14016.

@ShenFeng312
Copy link
Contributor

Hello everyone,

Through reading the source code of Spring Boot, I've observed that configuring monitoring exports seems to only require setting up XXXXMeterRegistry, such as InfluxMeterRegistry or OtlpMeterRegistry. In contrast, with Dubbo, our current initialization process involves setting up the complete AbstractMetricsReporter, which includes many complex implementations like addJvmMetrics, initCollectors, and even operations like registerDubboShutdownHook. I believe this approach is not quite reasonable. Instead, we should use composition over inheritance to clarify developers' tasks and implementation goals. This also aligns with the principle of least knowledge in design patterns. We should differentiate the management of metric collection/exposure and metric service lifecycle.

Additionally, we need to consider how to integrate and merge the monitoring of both Spring Boot Actuator and Dubbo in the same application. It's worth noting that this may not require specific attention within Dubbo, as we should differentiate between Dubbo and Dubbo-SpringBoot.

@glmapper
Copy link

I don't think implementing Spring Boot's implementation approach is a good idea;

1、Most components are integrated into Spring Boot, and autoconfiguration is a feature of Spring Boot. Therefore, component integration needs to meet the basic requirements of Spring Boot's automatic configuration and out-of-the-box usability, which relies on autoconfiguration to achieve.
2、Dubbo's automatic discovery mechanism is based on SPI implementation. Currently, most service discovery in Dubbo is also implemented through this mechanism, which aligns with the basic approach.
3、If there is a need to refactor the metrics part of the code, it may not necessarily be a refactoring but rather a better alignment with the usage habits of Spring Boot/Spring framework users. This can be achieved completely through Dubbo Spring Boot Starter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/sdk Related with apache/dubbo help wanted Everything needs help from contributors type/enhancement Everything related with code enhancement or performance
Projects
Status: Todo
Development

No branches or pull requests

6 participants