Skip to content

[feature] Support for Periodic Threshold preview. #3505

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

Merged
merged 9 commits into from
Jun 28, 2025

Conversation

Duansg
Copy link
Contributor

@Duansg Duansg commented Jun 24, 2025

What's changed?

Support for Periodic Threshold preview.

For details:

  1. Added Threshold -> Periodic Threshold preview, exception content support for internationalization
  2. Add anltr4 Look Ahead method to determine if parsing is finished.
  3. Add new test cases/fix test cases and go through the history of test cases successfully.

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Add or update API

  • I have added the necessary e2e tests and all cases have passed.
iShot_2025-06-24_20 15 55 iShot_2025-06-24_20 21 25

1、andOp
and

2、compareOp
0

3、orOp
OR

@MasamiYui
Copy link
Member

looks great!

@Duansg
Copy link
Contributor Author

Duansg commented Jun 24, 2025

looks great!

hh, review it for me if you have time. 😉

@MasamiYui
Copy link
Member

@tomsun28 @bigcyy What are your thoughts on this preview feature? Personally, From a UI perspective, as an common user, I might not fully grasp the meaning conveyed here, such as inner elements like __value__

@Duansg
Copy link
Contributor Author

Duansg commented Jun 25, 2025

@tomsun28 @bigcyy What are your thoughts on this preview feature? Personally, From a UI perspective, as an common user, I might not fully grasp the meaning conveyed here, such as inner elements like __value__

I don't think there is anything to worry about for the following reasons.

  1. If __value__ doesn't make sense, we can change it to value.
  2. If value is not understandable, then I would suggest the user to read the official documentation.

@bigcyy
Copy link
Member

bigcyy commented Jun 25, 2025

Looks good, but would it be better to display it as shown in the picture? My reasons are as follows:
Due to the 'and' set operations, if a structured table is used, the table header might be very long. Perhaps JSON would be more suitable.

IMG_4561

@Duansg
Copy link
Contributor Author

Duansg commented Jun 25, 2025

Looks good, but would it be better to display it as shown in the picture? My reasons are as follows: Due to the 'and' set operations, if a structured table is used, the table header might be very long. Perhaps JSON would be more suitable.

IMG_4561

That's good advice, I'll have time to deal with it tomorrow.

@MasamiYui
Copy link
Member

MasamiYui commented Jun 26, 2025

@Duansg How about adding a marker to the corresponding row if it can trigger an alert?

@Duansg
Copy link
Contributor Author

Duansg commented Jun 26, 2025

How about adding a marker to the corresponding row if it can trigger an alert?

@MasamiYui Sorry, I didn't quite get your point, could you expand on it. :)

@MasamiYui
Copy link
Member

MasamiYui commented Jun 26, 2025

Like this:

image

__VALUE__ == null -> No

__VALUE__ != null -> YES

@Duansg
Copy link
Contributor Author

Duansg commented Jun 26, 2025

Like this:

image `__VALUE__ == null` -> No

__VALUE__ != null -> YES

@MasamiYui Emmm, this is indeed in line with the current design, but I think we should not add this description for the time being, for the following reasons

  1. After our last discussion, I still firmly believe that we must ensure the integrity and scalability of the parser and ensure consistency with the promql semantics, that is, abandon the judgment of value == null and replace it with metrics == null. Otherwise, adding hertzbeat's unique judgment to the parser will make the parser very difficult to maintain and the complexity of achieving semantic correctness.
  2. If the above functional effects are achieved, the result of the preview function will be what you see is what you get, just like the search in the prometheus ui, so there is no need for a special mark to tell the user whether this item is hit, otherwise if a large amount of missed data is a terrible rendering function point.

Therefore, I think that it is enough to achieve the effect of basic preview at present. After we complete the relevant logic of the parser in the future, we will come back to see if there are other optimization points for this content. What do you think?

@Duansg
Copy link
Contributor Author

Duansg commented Jun 26, 2025

Looks good, but would it be better to display it as shown in the picture? My reasons are as follows: Due to the 'and' set operations, if a structured table is used, the table header might be very long. Perhaps JSON would be more suitable.

IMG_4561

@MasamiYui @bigcyy

Sorry friends, I'm busy with work recently. I will continue to improve this PR tomorrow. If you are all free at that time, please review it for me. Thanks. 😅

@bigcyy
Copy link
Member

bigcyy commented Jun 26, 2025

  1. I still firmly believe that we must ensure the integrity and scalability of the parser and ensure consistency with the promql semantics

I agree, see #3496 .

@Duansg
Copy link
Contributor Author

Duansg commented Jun 28, 2025

1、orOp
OR
2、andOp
and
3、comOp
com
3、error display
error
5、unlessOp
unless
4、metrics data is empty
iShot_2025-06-28_17 03 27

@Duansg Duansg requested a review from MasamiYui June 28, 2025 10:49
Copy link
Member

@MasamiYui MasamiYui left a comment

Choose a reason for hiding this comment

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

LGTM.

@MasamiYui MasamiYui merged commit e16d6cb into apache:master Jun 28, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants