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

add logs for home az monitor #1899

Merged
merged 5 commits into from
Apr 11, 2023
Merged

add logs for home az monitor #1899

merged 5 commits into from
Apr 11, 2023

Conversation

ZetaoZhuang
Copy link
Contributor

Reason for Change:

Issue Fixed:

Requirements:

Notes:

@ZetaoZhuang ZetaoZhuang requested a review from a team as a code owner April 6, 2023 22:38
@ZetaoZhuang ZetaoZhuang requested review from thatmattlong and ashvindeodhar and removed request for a team April 6, 2023 22:38
h.updateCacheValue(resp)
}

// logHomeAzResponse logs HomeAz Response
func (h *HomeAzMonitor) logHomeAzResponse(resp cns.GetHomeAzResponse) {
Copy link
Member

Choose a reason for hiding this comment

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

Hey Zetao, thinking more on this, I think we should log the HomeAzResponse everytime ( not just when it gets updated ). That's because, we may lose the AZ log if it doesn't change ( which it shouldn't ) for long time. Having that log would be beneficial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is right. I will update this

Copy link
Member

Choose a reason for hiding this comment

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

we don't need the func body below. We should just print the resp struct here.

Copy link
Contributor Author

@ZetaoZhuang ZetaoZhuang Apr 10, 2023

Choose a reason for hiding this comment

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

but in this way we can log it as error when there have errors in refreshing home az. it makes more flexible to set alerts in future.

Copy link
Member

Choose a reason for hiding this comment

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

I would keep the logging simple. Since we already have logic in DNC to identify mismatch on the home AZ we can add alerting there

@ZetaoZhuang ZetaoZhuang enabled auto-merge (squash) April 10, 2023 19:48
@ZetaoZhuang ZetaoZhuang merged commit 5f89e37 into master Apr 11, 2023
@ZetaoZhuang ZetaoZhuang deleted the log_cached_homeaz_value branch April 11, 2023 02:15
@smittal22 smittal22 added the AZR AZR related PR label Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AZR AZR related PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants