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

[LatencyMonitor] Decouple sending of ICMP probes and latency reporting #6812

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

faheem047
Copy link

@faheem047 faheem047 commented Nov 15, 2024

Fixes #6570

Separated the process of sending probes and reporting the latency to the response of send probes

Reporting frequency handled by a conditional block where minimum report interval is set to 10s with added jitter to avoid synchronization

I sincerely seek your guidance in resolving this issue and would greatly value your feedback and insights on the proposed changes.

Decoupled the sending of probes from the latency reporting in the NodeLatencyMonitor
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

thanks for your PR
please refer to the appropriate issue in the commit message and PR description using Fixes #<issue number>

pkg/agent/monitortool/monitor.go Outdated Show resolved Hide resolved
updateReportTicker := func(interval time.Duration) {
// Set minimum reporting interval to 10 seconds if needed
reportInterval := interval
if reportInterval < 10*time.Second{
Copy link
Contributor

Choose a reason for hiding this comment

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

define a top-level constant for the minimum reporting interval

Copy link
Author

@faheem047 faheem047 Nov 19, 2024

Choose a reason for hiding this comment

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

I have done the needful changes as asked well your review required in this regard

@faheem047 faheem047 changed the title Updated monitor.go Decoupled the process of Sennding ICMP probes and latency reporting and added delay before reporting Nov 19, 2024
@antoninbas antoninbas changed the title Decoupled the process of Sennding ICMP probes and latency reporting and added delay before reporting [LatencyMonitor] Decouple sending of ICMP probes and latency reporting Nov 19, 2024
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

The code changes look good to me, and I have approved running the github workflows for the PR. Some changes to unit tests may be required. Did you run the unit tests for this package locally?

@antoninbas
Copy link
Contributor

The code changes look good to me, and I have approved running the github workflows for the PR. Some changes to unit tests may be required. Did you run the unit tests for this package locally?

Confirmed that unit tests are failing in CI

@faheem047
Copy link
Author

faheem047 commented Nov 20, 2024

The code changes look good to me, and I have approved running the github workflows for the PR. Some changes to unit tests may be required. Did you run the unit tests for this package locally?

Confirmed that unit tests are failing in CI

Greetings,
Let me review the unit tests and make the necessary changes accordingly.
P.S. I might need some guidance along the way.
If possible, I will also run the tests locally to verify.

@faheem047
Copy link
Author

@antoninbas could you please guide me along what changes i have to do in monitor_test.go file in accordance to changes i have made in monitor.go a little refrerence would be helpful

@antoninbas
Copy link
Contributor

@antoninbas could you please guide me along what changes i have to do in monitor_test.go file in accordance to changes i have made in monitor.go a little refrerence would be helpful

The first step is to run go test ./pkg/agent/monitortool locally and look for which unit tests are failing. Then you need to update the tests accordingly to account for your changes. These unit tests use a "fake clock" to simulate the passing of time. You will need to change how the fake clock advances and maybe add a few assertions to validate things. I suggest reading the code for the failing tests before making any change, to make sure you understand the flow of the test and the existing assertions.

codephile1 and others added 2 commits January 5, 2025 15:25
Now all test are running except test at 712 line number of monitor_test.go
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

please leave a comment when you are ready and you want to request a review for us (or use the Github request review functionality).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's bad practice to check in a binary file to source control

It's great if you want to use act locally (even though you should not need it for this change), but please do not check in the act folder.

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.

Decouple the sending of probes from the latency reporting in the NodeLatencyMonitor
3 participants