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

Move MQE codes to independent Maven module inoap-server. #11176

Merged
merged 4 commits into from
Aug 5, 2023

Conversation

wankai123
Copy link
Member

@wankai123 wankai123 commented Aug 4, 2023

Since we plan to use MQE in other places(such as Alarm), I moved the relevant codes to the independent Maven module, to make it easier to depend on.

@wankai123 wankai123 added the backend OAP backend related. label Aug 4, 2023
@wankai123 wankai123 added this to the 9.6.0 milestone Aug 4, 2023
@sonatype-lift
Copy link

sonatype-lift bot commented Aug 4, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

wu-sheng
wu-sheng previously approved these changes Aug 4, 2023
@kezhenxu94
Copy link
Member

kezhenxu94 commented Aug 4, 2023

Since we plan to use MQE in other places(such as Alarm), I moved the relevant codes to server-core, to make it easier to depend on.

Can you elaborate (because I have no more context, only the PR description):

  • How/Why we would use MQE in alarm?
  • Did you try anything else that is "not easier" to depend on?
  • Why can't we make the MQE parser a dedicated module?

Asking because it sounds nonsense to move everything into the core module just to make it "easier" to depends on, although the current modularization is a mess, I hope not to make it worse

@wu-sheng
Copy link
Member

wu-sheng commented Aug 4, 2023

We are making the new alerting core. Totally changed.
We would do static configurations, instead, we would apply MQE(using metrics with windows as datasources, like a memory table) to query the metrics and compare with the threshold.
Typically, like this, exp=count(service_sla < 95) > 3 duration=10m, meaning 3 mins in the past 10 mins, the success rate is less than 95.
Also, the composite rules could be covered as well.

@wu-sheng
Copy link
Member

wu-sheng commented Aug 4, 2023

About depending on, we need an abstract data source concept(query or alerting core's memory windows) for feeding MQE.

We had a discussion about whether we need a maven module, but it seems either is fine. But an OAP-level module seems unnecessary. The MQE engine implementations, for now, can't extend w/o code changes

Comment on lines 209 to 210
@Override
public abstract ExpressionResult visitMetric(MQEParser.MetricContext ctx);
Copy link
Member

Choose a reason for hiding this comment

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

@kezhenxu94 From my reading/understanding, this is the extension point.
MQE on Query API implementation would pick query DAOs to access data from the database.
Alerting kernel will provide another implementation on this, to read data from time window based metric data.

@kezhenxu94
Copy link
Member

I still don't see why MQE-related codes should go into the core module, from the point of MQE, you might say "hey we have 10000 other modules that depends on MQE and we need to reuse it", that is a perfect reason why MQE MUST NOT be in the query plugin, but it's not a reason why it should go into the core module. To me it's not only about extension/reuse codes, it's also about organizing code, code structure, readability for new contributors, and sample for future contributors, for example, how do you persuade a contributor not to put random codes into the core module if they said "I do this by referring to the existing codes, and MQE did this".

One more thing, adding more codes into the core module does not only increase the granularity of the core module, but also introduces some transitive dependencies that can not be easily removed if anyone doesn't use the MQE query, and adds extra workload if anyone want to customize/extend the MQE features (e.g. add operators), they have to override the entire core module.

@wu-sheng
Copy link
Member

wu-sheng commented Aug 4, 2023

As I replied, I would have no interest to argue it should be in a maven module or not. I just said it should not be an OAP module, that's all. Because it doesn't take a responsibility of an independent feature.

You don't need to convince me. You asked for a context, so, I provided.

You and @wankai123 could choose your preference. I am fine for both ways.

@wu-sheng
Copy link
Member

wu-sheng commented Aug 4, 2023

If you want to separate codes, create a maven module, called mqe-lib for code reusing.

@wankai123
Copy link
Member Author

OK, I'll put them in a new maven module.

@wankai123
Copy link
Member Author

How about creating a new module in server-library and moving MQE into it? @kezhenxu94

@kezhenxu94
Copy link
Member

How about creating a new module in server-library and moving MQE into it? @kezhenxu94

Sounds perfect to me

@wankai123 wankai123 changed the title Move MQE codes to server-core. Move MQE codes to independent Maven module inoap-server. Aug 5, 2023
@wankai123 wankai123 requested a review from kezhenxu94 August 5, 2023 02:28
@wu-sheng wu-sheng merged commit a4c7705 into apache:master Aug 5, 2023
@wankai123 wankai123 deleted the move-mqe branch August 7, 2023 06:00
kezhenxu94 pushed a commit to kezhenxu94/skywalking that referenced this pull request Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants