eventmesh icon indicating copy to clipboard operation
eventmesh copied to clipboard

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

Open mxsm opened this issue 2 years ago • 16 comments

Fixes #3430

Motivation

  1. 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.
  2. The version upgrade brings new specifications and features, requiring code refactoring.
  3. 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/)
  4. Optimize and refactor the related code logic to make the code clearer.

Modifications

Refactor eventmesh-metrics-prometheus module image

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

mxsm avatar Mar 13 '23 16:03 mxsm

Codecov Report

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

:exclamation: 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

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

codecov[bot] avatar Mar 14 '23 02:03 codecov[bot]

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 avatar Mar 14 '23 16:03 mxsm

I will refactor this pr code and resubmit

mxsm avatar Apr 19 '23 11:04 mxsm

@xwm1992 PTAL~

mxsm avatar May 06 '23 08:05 mxsm

@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 avatar Jul 05 '23 15:07 Pil0tXia

@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 avatar Jul 05 '23 15:07 pandaapo

@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 avatar Jul 05 '23 16:07 mxsm

@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.

mxsm avatar Jul 05 '23 16:07 mxsm

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?

pandaapo avatar Jul 05 '23 23:07 pandaapo

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.

Pil0tXia avatar Jul 06 '23 01:07 Pil0tXia

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.

mxsm avatar Jul 17 '23 15:07 mxsm

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

image

Pil0tXia avatar Aug 10 '23 13:08 Pil0tXia

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

image

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

mxsm avatar Aug 10 '23 14:08 mxsm

Any progress we can make here?

Pil0tXia avatar Jan 11 '24 12:01 Pil0tXia

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.

mxsm avatar Jan 11 '24 13:01 mxsm

@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 avatar Jan 24 '24 15:01 mxsm

This pr will be closed, the new is #4840

mxsm avatar Apr 14 '24 15:04 mxsm