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

vmalert-tool: implement unittest #4789

Merged
merged 8 commits into from Oct 13, 2023
Merged

vmalert-tool: implement unittest #4789

merged 8 commits into from Oct 13, 2023

Conversation

Haleygo
Copy link
Collaborator

@Haleygo Haleygo commented Aug 7, 2023

address #2945
There are two requirements to perform unittest:

  1. mock an isolated VictoriaMetrics instance, can insert like vminsert, storing like vmstorage, quering like vmselect. Split or mock those functions can be very expensive, so I think we still need to reuse them by importing their packages.
  2. evaluate rules exactly like vmalert.

vmalert can't implement this like #4734 said.
vmctl used to be considered, but there are unexposed parts in vmalert, so it's not be implemented at that time. Now vmctl is considered only for migration stuff.

update:
After discussion, we are adding a new tool named vmalert-tool, it's only used for unittest now.

This pull request does two things:

  1. split package rule under vmalert, expose needed objects. After this, vmalert should become more functionality, other components can use vmalert library to do rule evaluation.
    Implement rule evaluation for vmsingle in the future maybe, could simplify the combination of vmsingle+vmalert, mentioned in Add remote_write support to victoria metrics (single) #1645 (comment)

  2. create new vmalert-tool to bring unittest feature in vmalert: init unit test #4596.

@Haleygo Haleygo marked this pull request as draft August 16, 2023 12:54
Copy link
Collaborator

@hagen1778 hagen1778 left a comment

Choose a reason for hiding this comment

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

I think we're going in the right direction. But we should try to split responsibility between packages in more clear way. Let's agree on the following:

  1. main.go is responsible for initial validation of the given params and for initing components like: datasource, remote read, remote write, notifier.
  2. main.go is responsible for scheduling rules execution via private component manager
  3. Manager supposed to operate on the Groups level, where Group is a part of rule package
  4. Rules package supposed to expose Group, Alerting and Recording structs. The Rule interface can be private, as main.go should use Group to exec rules.
  5. web component is a part of main package and should operating based on the public structs from the rule package. Methods like RuleToAPI could be private and belong to main package only.
  6. Unit tests should operate on rules execution via Group, the same way as in p3. It could be needed to add an extra method to a Group struct which would evaluate all the rules on the call. Additionally to Start method, which only schedules the evaluation.

This separation supposed to isolate responsibility of components and keep private fields private. It should also provide the same way for rules execution for main.go and for unit tests.

One extra comment is that for unittests it might be better to create a new tool instead of integrating into vmctl. It is right that vmctl was initially meant to become a cli tool for VM. But historically, vmctl is responsible for migration only. Creating a vmalert-tool could be easier to grasp and argue about for users. Probably, we'll move replay mode to vmalert-tool after that.

Let me know wdyt.

@Haleygo
Copy link
Collaborator Author

Haleygo commented Aug 18, 2023

But historically, vmctl is responsible for migration only.

@hagen1778
Yeah, it was a bit of strange when added this subcmd to vmctl which seems all about data processing.
But having a second tool might be little inconvenient for users, maybe we can embed it into vmalert in the future and use it in web like #4691.
And I don't have better idea than vmalert-tool now, will create it)

@Haleygo Haleygo changed the title vmctl: add rule-unittest vmalert-tool: implement unittest Aug 27, 2023
@Haleygo Haleygo force-pushed the vmctl-unittest-rule branch 4 times, most recently from 8d5f502 to a76c728 Compare August 27, 2023 15:15
@Haleygo Haleygo marked this pull request as ready for review August 27, 2023 15:16
@Haleygo Haleygo force-pushed the vmctl-unittest-rule branch 3 times, most recently from 6c51b86 to f4bb876 Compare September 14, 2023 07:36
app/vmalert/rule/alerting.go Outdated Show resolved Hide resolved
app/vmalert/rule/alerting.go Outdated Show resolved Hide resolved
app/vmalert/rule/group.go Outdated Show resolved Hide resolved
app/vmalert/rule/group.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Attention: 546 lines in your changes are missing coverage. Please review.

Comparison is base (4f4bc51) 60.37% compared to head (6e0db01) 60.21%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4789      +/-   ##
==========================================
- Coverage   60.37%   60.21%   -0.17%     
==========================================
  Files         389      397       +8     
  Lines       73142    74019     +877     
==========================================
+ Hits        44163    44572     +409     
- Misses      26556    26957     +401     
- Partials     2423     2490      +67     
Files Coverage Δ
app/vmalert/replay.go 42.50% <100.00%> (-20.34%) ⬇️
app/vmalert/rule/utils.go 96.59% <ø> (ø)
app/vmalert/main.go 39.08% <0.00%> (ø)
app/vmalert/manager.go 88.07% <94.73%> (+0.64%) ⬆️
app/vmalert/web.go 66.66% <85.71%> (ø)
app/vmalert/web.qtpl 72.77% <76.19%> (+1.45%) ⬆️
lib/promutils/duration.go 57.57% <0.00%> (-10.29%) ⬇️
app/vmalert/rule/recording.go 77.37% <71.42%> (ø)
app/vmalert-tool/unittest/recording.go 71.42% <71.42%> (ø)
app/vmalert/remotewrite/debug_client.go 66.07% <66.07%> (ø)
... and 11 more

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

docs/vmalert-tool.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@hagen1778 hagen1778 left a comment

Choose a reason for hiding this comment

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

Almost ready now! Thanks for hard work!

app/vmalert/rule/alerting.go Outdated Show resolved Hide resolved
app/vmalert/rule/alerting.go Outdated Show resolved Hide resolved
app/vmalert/rule/alerting.go Outdated Show resolved Hide resolved
app/vmalert/__debug_bin Outdated Show resolved Hide resolved
app/vmalert/main.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@hagen1778 hagen1778 left a comment

Choose a reason for hiding this comment

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

LGTM!

@hagen1778 hagen1778 merged commit dc28196 into master Oct 13, 2023
9 of 10 checks passed
@hagen1778 hagen1778 deleted the vmctl-unittest-rule branch October 13, 2023 11:54
valyala pushed a commit that referenced this pull request Oct 16, 2023
1. split package rule under /app/vmalert, expose needed objects
2. add vmalert-tool with unittest subcmd

#2945
valyala added a commit that referenced this pull request Oct 16, 2023
…seTime() to app/vmalert-tool/unittest.durationToTime()

The ParseTime() function looks strange, since it converts relative duration to absolute time since Unix Epoch.
In most scenarios such a conversion is used by mistake.

It is better to do not expose such a function for public use and hide it inside the package where it is needed,
e.g. inside app/vmalert-tool/unittest.

This is a follow-up for dc28196
Updates #2945
Updates #4789
valyala added a commit that referenced this pull request Oct 16, 2023
…seTime() to app/vmalert-tool/unittest.durationToTime()

The ParseTime() function looks strange, since it converts relative duration to absolute time since Unix Epoch.
In most scenarios such a conversion is used by mistake.

It is better to do not expose such a function for public use and hide it inside the package where it is needed,
e.g. inside app/vmalert-tool/unittest.

This is a follow-up for dc28196
Updates #2945
Updates #4789
AndrewChubatiuk pushed a commit to AndrewChubatiuk/VictoriaMetrics that referenced this pull request Nov 15, 2023
1. split package rule under /app/vmalert, expose needed objects
2. add vmalert-tool with unittest subcmd

VictoriaMetrics#2945
AndrewChubatiuk pushed a commit to AndrewChubatiuk/VictoriaMetrics that referenced this pull request Nov 15, 2023
…seTime() to app/vmalert-tool/unittest.durationToTime()

The ParseTime() function looks strange, since it converts relative duration to absolute time since Unix Epoch.
In most scenarios such a conversion is used by mistake.

It is better to do not expose such a function for public use and hide it inside the package where it is needed,
e.g. inside app/vmalert-tool/unittest.

This is a follow-up for dc28196
Updates VictoriaMetrics#2945
Updates VictoriaMetrics#4789
@valyala
Copy link
Collaborator

valyala commented Nov 15, 2023

FYI, this pull request has been included in v1.95.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants