[ISSUE #3430]Refactor eventmesh-metrics-prometheus module #3446

mxsm · 2023-03-13T16:02:31Z

Fixes #3430

Motivation

The current project is using a lower version of OpenTelemetry components, version 1.3.0, while the latest version is 1.24.0. The related versions need to be upgraded.
The version upgrade brings new specifications and features, requiring code refactoring.
The instrumentation of metrics needs to be adjusted accordingly based on the related semantic conversion specifications of OpenTelemetry. (https://opentelemetry.io/docs/reference/specification/metrics/semantic_conventions/)
Optimize and refactor the related code logic to make the code clearer.

Modifications

Refactor eventmesh-metrics-prometheus module

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
If a feature is not applicable for documentation, explain why?
If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

codecov · 2023-03-14T02:05:12Z

Codecov Report

Merging #3446 (9320d02) into master (3678c23) will decrease coverage by 0.54%.
The diff coverage is 2.46%.

❗ Current head 9320d02 differs from pull request most recent head 005b880. Consider uploading reports for the commit 005b880 to get more accurate results

@@             Coverage Diff              @@
##             master    #3446      +/-   ##
============================================
- Coverage     17.82%   17.29%   -0.54%     
- Complexity     1514     1518       +4     
============================================
  Files           604      633      +29     
  Lines         25559    26413     +854     
  Branches       2400     2414      +14     
============================================
+ Hits           4556     4568      +12     
- Misses        20564    21403     +839     
- Partials        439      442       +3

Files Changed	Coverage Δ
...rc/main/java/org/apache/eventmesh/common/Pair.java	`0.00% <0.00%> (ø)`
...rg/apache/eventmesh/common/enums/ProtocolType.java	`0.00% <0.00%> (ø)`
.../apache/eventmesh/metrics/api/MetricsRegistry.java	`0.00% <0.00%> (ø)`
...he/eventmesh/metrics/api/model/AbstractMetric.java	`0.00% <0.00%> (ø)`
...rics/api/model/AbstractObservableDoubleMetric.java	`0.00% <0.00%> (ø)`
...etrics/api/model/AbstractObservableLongMetric.java	`0.00% <0.00%> (ø)`
...sh/metrics/api/model/AbstractObservableMetric.java	`0.00% <0.00%> (ø)`
...ventmesh/metrics/api/model/AbstractSyncMetric.java	`0.00% <0.00%> (ø)`
...entmesh/metrics/api/model/DoubleCounterMetric.java	`0.00% <0.00%> (ø)`
...tmesh/metrics/api/model/DoubleHistogramMetric.java	`0.00% <0.00%> (ø)`
... and 77 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mxsm · 2023-03-14T16:02:31Z

I will try fix the License Check not pass and resubmit later, But reviewer can do review work first if you have time to this work. thanks

mxsm · 2023-04-19T11:07:56Z

I will refactor this pr code and resubmit

mxsm · 2023-05-06T08:01:50Z

@xwm1992 PTAL~

Pil0tXia · 2023-07-05T15:19:50Z

@pandaapo @horoc
Refactoring the org.apache.eventmesh.runtime.admin package into the eventmesh-admin module is one of the pending tasks on our to-do list. It is advisable to merge this PR before the refactoring takes place, in order to avoid potential conflicts. PTAL at your convenience. 😉

pandaapo · 2023-07-05T15:35:46Z

@Pil0tXia I'm sorry about that I'm not familiar with this module for now, and in fact I do not have permission to perform an effective approving or decide merging a PR.

If you are worried about potential conflicts during the refactoring of "eventmesh admin", you can try asking for advice from maintainers. They may advise you to refactor first.

mxsm · 2023-07-05T16:25:38Z

@pandaapo @horoc Refactoring the org.apache.eventmesh.runtime.admin package into the eventmesh-admin module is one of the pending tasks on our to-do list. It is advisable to merge this PR before the refactoring takes place, in order to avoid potential conflicts. PTAL at your convenience. 😉

@Pil0tXia There will be a biweekly meeting on Thursday where I can bring up this issue. Since it involves some refactoring, I will explain the problem during the meeting. Meanwhile, I will provide the corresponding documentation to the community as soon as possible. Additionally, everyone can help by reviewing the code. Let's aim to have the community merge this pull request as soon as possible.

mxsm · 2023-07-05T16:29:23Z

@Pil0tXia I'm sorry about that I'm not familiar with this module for now, and in fact I do not have permission to perform an effective approving or decide merging a PR.

If you are worried about potential conflicts during the refactoring of "eventmesh admin", you can try asking for advice from maintainers. They may advise you to refactor first.

@pandaapo The refactoring of the admin on his side is not a major issue, and my pull request may be merged before his. He can simply rebase his branch onto the master branch when the time comes, but this process may involve resolving conflicts. The overall changes related to observability are quite significant. The main upgrades are due to version changes, resulting in differences from the previous approach. There is also a re-design of the top-level interface, among other things.

pandaapo · 2023-07-05T23:23:58Z

He can simply rebase his branch onto the master branch when the time comes, but this process may involve resolving conflicts.

Do you mean that the development of refactoring "admin" can be concurrent with the rest works of this PR?

Pil0tXia · 2023-07-06T01:56:10Z

Concurrent work is technically feasible, while refactoring after this PR is merged will have fewer conflicts.

I will start refactoring maybe 1 month later. Not so urgent.

mxsm · 2023-07-17T15:19:49Z

chinese document about metrics. The documentation will be updated on the official website as part of the code merge and subsequent PR submission. Additionally, further optimizations will be made to improve the documentation.

Pil0tXia · 2023-08-10T13:41:45Z

@mxsm Hi~ EventMesh master often throws this exception during normal operation, a feedback to you~

mxsm · 2023-08-10T14:38:48Z

@mxsm Hi~ EventMesh master often throws this exception during normal operation, a feedback to you~

Thanks for your feedback, I will try to find it and fix

…not download some jar package

Pil0tXia · 2024-01-11T12:00:28Z

Any progress we can make here?

mxsm · 2024-01-11T13:20:11Z

Any progress we can make here?

@Pil0tXia
I will resolve the conflicts. Can you help review the code? I've provided explanations for the code previously.

Pil0tXia

Overall, I didn't see any major vulnerabilities, but there are quite a few code additions related to metrics in the EventMeshServer, which doesn't seem very maintainable.

Additionally, the org.apache.eventmesh.runtime.metrics and org.apache.eventmesh.metrics.api.model packages could be made clearer.

Pil0tXia · 2024-01-12T22:46:51Z

eventmesh-common/src/main/java/org/apache/eventmesh/common/MetricsConstants.java

+package org.apache.eventmesh.common;
+
+
+public abstract class MetricsConstants {
+
+ private MetricsConstants() {
+
+ }


Maybe decorating this constant with class is enough, instead of abstract class.

The usages of this constant are all in metrics-plugin module, so perhaps placing this class in metrics-plugin module will be better.

Pil0tXia · 2024-01-12T22:49:08Z

eventmesh-common/src/main/java/org/apache/eventmesh/common/Pair.java

+package org.apache.eventmesh.common;
+
+public class Pair<Left, Right> {
+
+ private Left left;


We have two Pair classes in our project:

org.apache.eventmesh.common.Pair

org.apache.eventmesh.runtime.common.Pair

The usages of the latter one can be unified to the former one.

Pil0tXia · 2024-01-12T22:52:07Z

...entmesh-metrics-api/src/main/java/org/apache/eventmesh/metrics/api/model/AbstractMetric.java

+package org.apache.eventmesh.metrics.api.model;
+
+public abstract class AbstractMetric implements Metric {
+


The classes under org.apache.eventmesh.metrics.api.model are too many and their inheritance relationship seems a little messy.

Pil0tXia · 2024-01-12T23:05:07Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/EventMeshHttpBootstrap.java

 eventMeshHttpServer = new EventMeshHTTPServer(eventMeshServer, eventMeshHttpConfiguration);
+ eventMeshHttpServer.init();
 }


Is it necessary to init eventMeshHttpServer here?

Pil0tXia · 2024-01-12T23:13:21Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/boot/EventMeshServer.java

 }
+ List<String> metricsPluginTypes = configuration.getEventMeshMetricsPluginType();
+ if (CollectionUtils.isNotEmpty(metricsPluginTypes)) {
+ List<MetricsRegistry> metricsRegistries = metricsPluginTypes.stream().map(metric -> MetricsPluginFactory.getMetricsRegistry(metric))
+ .collect(Collectors.toList());
+ eventMeshMetricsManager = new EventMeshMetricsManager(metricsRegistries);
+ }


I think that maybe it is not a good design to place a list of metrics plugin types here. Is there a similar way to init metrics like other plugins (L79~L83)?

Pil0tXia · 2024-01-12T23:20:17Z

.../main/java/org/apache/eventmesh/runtime/core/protocol/http/processor/HeartBeatProcessor.java

+ if (StringUtils.isBlank(heartbeatRequestHeader.getIdc())
+ || StringUtils.isBlank(heartbeatRequestHeader.getPid())
+ || !StringUtils.isNumeric(heartbeatRequestHeader.getPid())
+ || StringUtils.isBlank(heartbeatRequestHeader.getSys())) {
+ responseEventMeshCommand = asyncContext.getRequest().createHttpCommandResponse(
+ heartbeatResponseHeader,
+ HeartbeatResponseBody.buildBody(EventMeshRetCode.EVENTMESH_PROTOCOL_HEADER_ERR.getRetCode(),
+ EventMeshRetCode.EVENTMESH_PROTOCOL_HEADER_ERR.getErrMsg()));
+ asyncContext.onComplete(responseEventMeshCommand);


It seems that the main change here is the invocation of createHttpCommandResponse. What are the advantages of the validation logic here compared to the original? Bringing a more comprehensive validation logic?

Pil0tXia · 2024-01-12T23:31:06Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/metrics/MetricInstrumentUnit.java

+
+public abstract class MetricInstrumentUnit {
+
+ private MetricInstrumentUnit() {
+
+ }


We can put this constant class into constant package together with MonitorMetricConstants.

Pil0tXia · 2024-01-12T23:32:45Z

...esh-runtime/src/main/java/org/apache/eventmesh/runtime/metrics/tcp/TcpMetricsCalculator.java

+@Slf4j
+class TcpMetricsCalculator {
+
+ private static final int period = 30 * 1000;


Why TCP needs a "Calculator" comparing to HTTP and gRPC?

Pil0tXia · 2024-01-12T23:34:13Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/metrics/MetricsManager.java

+/**
+ * metric manager interface
+ */
+
+/**
+ * MetricsManager is an interface for managing metrics.
+ */
+public interface MetricsManager {


duplicate javadoc

Pil0tXia · 2024-01-12T23:37:36Z

eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/metrics/GeneralMetricsManager.java

+/**
+ * Managing general metrics.
+ */
+@UtilityClass
+public class GeneralMetricsManager {


The naming of GeneralMetricsManager is a little confusing comparing with MetricsManager. How about putting them in different packages and rename it to a more suitable name?

mxsm · 2024-01-24T15:46:34Z

@Pil0tXia This PR has been open for too long, and many code changes make resolving conflicts during refactoring challenging. I will submit a new PR based on the latest code. The overall design will still be based on the current PR, and I will address the issues you mentioned above one by one.

mxsm · 2024-04-14T15:44:46Z

This pr will be closed, the new is #4840

mxsm force-pushed the eventmesh-refactor-metrics-3430 branch from b27fee6 to 3a17b8b Compare March 15, 2023 15:33

mxsm force-pushed the eventmesh-refactor-metrics-3430 branch from 3a17b8b to 34c25f0 Compare April 18, 2023 15:28

mxsm closed this Apr 19, 2023

mxsm reopened this Apr 25, 2023

mxsm force-pushed the eventmesh-refactor-metrics-3430 branch from f68ab89 to 06dc8b7 Compare April 25, 2023 15:34

mxsm force-pushed the eventmesh-refactor-metrics-3430 branch from e987993 to f77cb70 Compare May 6, 2023 07:57

mxsm force-pushed the eventmesh-refactor-metrics-3430 branch 5 times, most recently from ce0aa44 to a5fca9d Compare July 12, 2023 15:38

mxsm force-pushed the eventmesh-refactor-metrics-3430 branch from 5824674 to 79324ad Compare July 17, 2023 13:44

mxsm force-pushed the eventmesh-refactor-metrics-3430 branch from 29c98d4 to 9856c70 Compare July 23, 2023 15:44

mxsm requested review from xwm1992 and pandaapo August 1, 2023 03:31

This was referenced Aug 2, 2023

[ISSUE #4201] Do some code optimization.[metrics] #4296

Closed

Bump io.opentelemetry:opentelemetry-api from 1.3.0 to 1.27.0 #4115

Closed

mxsm added 8 commits August 27, 2023 22:37

update OpenTelemetry version to 1.24.0 and fix aliyun repository can …

191eba1

…not download some jar package

refactor metrics module

d89c6b7

fix code style

a0197ff

Fixed dependency check failure

f74951f

add code doc

c94558b

remove lombok @DaTa annotation

7d4be52

add general metrics into EventMeshHttpMetricsManager

30f3359

add grpc publish handle cost

005b880

mxsm force-pushed the eventmesh-refactor-metrics-3430 branch from 9856c70 to 005b880 Compare August 27, 2023 14:56

Pil0tXia reviewed Jan 12, 2024

View reviewed changes

mxsm closed this Apr 14, 2024

Pil0tXia mentioned this pull request Apr 14, 2024

[ISSUE #3430] Refactor eventmesh-metrics-prometheus module #4840

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE #3430]Refactor eventmesh-metrics-prometheus module #3446

[ISSUE #3430]Refactor eventmesh-metrics-prometheus module #3446

mxsm commented Mar 13, 2023 •

edited

Loading

codecov bot commented Mar 14, 2023 •

edited

Loading

mxsm commented Mar 14, 2023 •

edited

Loading

mxsm commented Apr 19, 2023

mxsm commented May 6, 2023

Pil0tXia commented Jul 5, 2023

pandaapo commented Jul 5, 2023 •

edited

Loading

mxsm commented Jul 5, 2023

mxsm commented Jul 5, 2023

pandaapo commented Jul 5, 2023

Pil0tXia commented Jul 6, 2023

mxsm commented Jul 17, 2023 •

edited

Loading

Pil0tXia commented Aug 10, 2023 •

edited

Loading

mxsm commented Aug 10, 2023

Pil0tXia commented Jan 11, 2024

mxsm commented Jan 11, 2024

Pil0tXia left a comment

Pil0tXia Jan 12, 2024

Pil0tXia Jan 12, 2024

Pil0tXia Jan 12, 2024

Pil0tXia Jan 12, 2024

Pil0tXia Jan 12, 2024

Pil0tXia Jan 12, 2024

Pil0tXia Jan 12, 2024

Pil0tXia Jan 12, 2024

Pil0tXia Jan 12, 2024

Pil0tXia Jan 12, 2024

mxsm commented Jan 24, 2024

mxsm commented Apr 14, 2024

		package org.apache.eventmesh.metrics.api.model;

		public abstract class AbstractMetric implements Metric {

[ISSUE #3430]Refactor eventmesh-metrics-prometheus module #3446

[ISSUE #3430]Refactor eventmesh-metrics-prometheus module #3446

Conversation

mxsm commented Mar 13, 2023 • edited Loading

Motivation

Modifications

Documentation

codecov bot commented Mar 14, 2023 • edited Loading

Codecov Report

mxsm commented Mar 14, 2023 • edited Loading

mxsm commented Apr 19, 2023

mxsm commented May 6, 2023

Pil0tXia commented Jul 5, 2023

pandaapo commented Jul 5, 2023 • edited Loading

mxsm commented Jul 5, 2023

mxsm commented Jul 5, 2023

pandaapo commented Jul 5, 2023

Pil0tXia commented Jul 6, 2023

mxsm commented Jul 17, 2023 • edited Loading

Pil0tXia commented Aug 10, 2023 • edited Loading

mxsm commented Aug 10, 2023

Pil0tXia commented Jan 11, 2024

mxsm commented Jan 11, 2024

Pil0tXia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxsm commented Jan 24, 2024

mxsm commented Apr 14, 2024

mxsm commented Mar 13, 2023 •

edited

Loading

codecov bot commented Mar 14, 2023 •

edited

Loading

mxsm commented Mar 14, 2023 •

edited

Loading

pandaapo commented Jul 5, 2023 •

edited

Loading

mxsm commented Jul 17, 2023 •

edited

Loading

Pil0tXia commented Aug 10, 2023 •

edited

Loading