Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 19, 2024
Merged

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

merged 3 commits into from
Apr 19, 2024

Conversation

mxsm
Copy link
Member

@mxsm mxsm commented Apr 14, 2024

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 mxsm mentioned this pull request Apr 14, 2024
Closed
@mxsm mxsm requested review from xwm1992 and Pil0tXia April 14, 2024 15:45
@mxsm mxsm force-pushed the eventmesh-refactor-metrics-3430-new branch from ef6dd44 to 8936c98 Compare April 14, 2024 16:04
@Pil0tXia Pil0tXia changed the title [ISSUE #3430]Refactor eventmesh-metrics-prometheus module [ISSUE #3430] Refactor eventmesh-metrics-prometheus module Apr 14, 2024
Pil0tXia
Pil0tXia reviewed Apr 14, 2024
View reviewed changes
Copy link
Member

@Pil0tXia Pil0tXia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read all the code from your last PR before. Is there any new commit that needs to be read between these two PRs? 😊

...entmesh-metrics-api/src/main/java/org/apache/eventmesh/metrics/api/model/AbstractMetric.java
Comment on lines +18 to +22
package org.apache.eventmesh.metrics.api.model;

public abstract class AbstractMetric implements Metric {

private InstrumentFurther further;
Copy link
Member

@Pil0tXia Pil0tXia Apr 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to extract implementation classes to a sub-package?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to put this in a subclass; this covers all implementations. Using an abstract class makes it easier for future extensions.

Copy link
Member

@Pil0tXia Pil0tXia Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to move files location without making changes to the class. The directory structure may like this:

model/
    counters/
        CounterA (Impl)
        CounterB (Impl)
    xxx/
        xxxA (Impl)
        xxxB (Impl)
Counter (Interface)
xxx (Interface)
xxxX (AbstractClass)
xxxY (AbstractClass)

It may be easier to find out how many interfaces and abstract classes are under this package and locate them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The model package is mainly used to store metrics. Personally, I don't think it's necessary to further classify the types of metrics. They can be uniformly placed in this package. Similarly, this package basically includes metrics that adhere to the existing OpenTelemetry specification. Being too granular makes the code look bloated.

@mxsm mxsm requested a review from Pil0tXia April 15, 2024 15:48
xwm1992
xwm1992 previously approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@xwm1992 xwm1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mxsm mxsm requested a review from xwm1992 April 17, 2024 14:21
Pil0tXia
Pil0tXia approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@Pil0tXia Pil0tXia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xwm1992 xwm1992 merged commit a899e54 into apache:master Apr 19, 2024
9 checks passed
@mxsm mxsm deleted the eventmesh-refactor-metrics-3430-new branch April 19, 2024 02:27
@xwm1992 xwm1992 added this to the 1.11.0 milestone Dec 18, 2024
xuhongjia pushed a commit to Deckers-Ohana/eventmesh that referenced this pull request Mar 13, 2025
@mxsm
[ISSUE apache#3430] Refactor eventmesh-metrics-prometheus module (apa…
3840091 
…che#4840)

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

* optimize code

* optimize code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xwm1992 xwm1992 xwm1992 approved these changes

@Pil0tXia Pil0tXia Pil0tXia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.11.0
Development

Successfully merging this pull request may close these issues.

[Enhancement] Refactor eventmesh-metrics-prometheus module
3 participants
@mxsm @xwm1992 @Pil0tXia