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.

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

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

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

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

wants to merge 3 commits into from

Conversation

faheem047
Copy link

@faheem047 faheem047 commented Nov 15, 2024 •
edited
Loading

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.

@faheem047
Updated monitor.go
b0cfe9c 
Decoupled the sending of probes from the latency reporting in the NodeLatencyMonitor
antoninbas
antoninbas reviewed Nov 18, 2024
View reviewed changes
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
pkg/agent/monitortool/monitor.go Outdated
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 •
edited
Loading

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
antoninbas
antoninbas reviewed Nov 19, 2024
View reviewed changes
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 •
edited
Loading

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.

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

@antoninbas antoninbas antoninbas left review comments

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

Decouple the sending of probes from the latency reporting in the NodeLatencyMonitor
2 participants
@faheem047 @antoninbas