Skip to content

Conversation

@ym
Copy link
Contributor

@ym ym commented Apr 3, 2025

  • add metrics for cloud connections
  • skip websocket client if net isn't up or time sync hasn't complete

@ym ym requested a review from adamshiervani April 3, 2025 17:08
@adamshiervani adamshiervani requested a review from Copilot April 3, 2025 17:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR aims to improve the websocket client functionality by adding Prometheus metrics for monitoring cloud connections and ensuring that the client only initiates when both the network is up and the system time is synchronized. Key changes include:

  • Introducing a new time synchronization function in ntp.go.
  • Adjusting the startup conditions in main.go for the websocket client.
  • Adding multiple Prometheus metrics and related reset logic in cloud.go.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
ntp.go Added a function to check if time sync is needed and introduced new state.
main.go Simplified websocket client startup logic by removing redundant checks.
cloud.go Introduced Prometheus metrics for cloud connection monitoring and updated client restart logic.
Comments suppressed due to low confidence (2)

ntp.go:73

  • [nitpick] Consider using the return value of isTimeSyncNeeded() to conditionally control the time sync process instead of discarding it, if this behavior is intended for decision-making.
isTimeSyncNeeded()

cloud.go:400

  • [nitpick] There is an inconsistency in how the cloud connection status metric is set: cloudResetMetrics(false) sets it to -1 while an error branch sets it to 0. Consider standardizing these values for clarity and consistency.
metricCloudConnectionStatus.Set(0)

@ym ym merged commit 1a26431 into dev Apr 3, 2025
5 checks passed
@IDisposable IDisposable deleted the chore/lazy-websocket branch September 22, 2025 23:42
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.

3 participants