NR-126476: update NTP interval check on error (#1699)

* NR-126476: move Offset interval call responsability to caller

* NR-126476: return error if ntp interval

* NR-126476: add test case for host sample

* feat: remove internal cached offset

Author: rogercoll

pkg/metrics/host.go

@@ -4,6 +4,7 @@
 package metrics
 
 import (
+	"errors"
 	"fmt"
 	"time"
 
@@ -17,6 +18,7 @@ type HostSample struct {
 
 type HostMonitor struct {
 	ntpMonitor NtpMonitor
+	ntpOffset  *float64 // cache for last offset value retrieved
 }
 
 type NtpMonitor interface {
@@ -38,11 +40,16 @@ func (m *HostMonitor) Sample() (*HostSample, error) {
 	if m.ntpMonitor != nil {
 		ntpOffset, err := m.ntpMonitor.Offset()
 		if err != nil {
-			syslog.WithError(err).Error("cannot get ntp offset")
+			// skip the error and use cached offset if interval error
+			if !errors.Is(err, ErrNotInInterval) {
+				syslog.WithError(err).Error("cannot get ntp offset")
+				m.ntpOffset = nil
+			}
 		} else {
 			seconds := ntpOffset.Seconds()
-			hostSample.NtpOffset = &seconds
+			m.ntpOffset = &seconds
 		}
+		hostSample.NtpOffset = m.ntpOffset
 	}
 
 	return hostSample, nil

pkg/metrics/host_test.go

@@ -0,0 +1,39 @@
+// Copyright New Relic Corporation. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package metrics
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestHostSample_CachedNtpOffset(t *testing.T) {
+	timeout := uint(5000)
+	interval := uint(15)
+	ntpMonitor := NewNtp([]string{"one"}, timeout, interval)
+	ntpMonitor.ntpQuery = ntpQueryMock([]ntpResp{
+		{
+			resp: buildValidNtpResponse(50 * time.Millisecond),
+			err:  nil,
+		},
+	}...)
+
+	hostMonitor := NewHostMonitor(ntpMonitor)
+
+	expectedOffset := (50 * time.Millisecond).Seconds()
+	expectedNtpSample := &expectedOffset
+
+	// valid ntp response
+	ntpMonitor.ntpQuery = ntpQueryMock([]ntpResp{{buildValidNtpResponse(50 * time.Millisecond), nil}}...)
+	sample, err := hostMonitor.Sample()
+	assert.Nil(t, err)
+	assert.Equal(t, expectedNtpSample, sample.NtpOffset)
+
+	// query inside interval should return cached value
+	sample, err = hostMonitor.Sample()
+	assert.Nil(t, err)
+	assert.Equal(t, expectedNtpSample, sample.NtpOffset)
+}

pkg/metrics/ntp.go

@@ -23,14 +23,14 @@ const (
 var (
 	ErrEmptyNtpHosts    = errors.New("ntp host list is empty")
 	ErrGettingNtpOffset = errors.New("cannot get ntp offset")
+	ErrNotInInterval    = errors.New("cannot query ntp servers offset inside interval")
 )
 
 type Ntp struct {
 	pool      []string
 	timeout   time.Duration // ntp request timeout in seconds
 	interval  time.Duration // ntp request interval in minutes
 	updatedAt time.Time     // last time the ntp offset was fetched
-	offset    time.Duration // cache for last offset value retrieved
 	now       func() time.Time
 	ntpQuery  func(host string, opt ntp.QueryOptions) (*ntp.Response, error)
 }
@@ -69,16 +69,22 @@ func guardInterval(interval uint) uint {
 	return interval
 }
 
+// Offset returns the Ntp servers offset.
 func (p *Ntp) Offset() (time.Duration, error) {
 	if len(p.pool) == 0 {
 		return 0, ErrEmptyNtpHosts
 	}
 
-	// We only query the servers once every p.interval
 	if p.now().Sub(p.updatedAt) < p.interval {
-		return p.offset, nil
+		// return error in case outside query interval
+		return 0, ErrNotInInterval
 	}
 
+	// update current interval even if error
+	defer func() {
+		p.updatedAt = p.now()
+	}()
+
 	var offsets []time.Duration
 
 	var ntpQueryErr error
@@ -113,9 +119,5 @@ func (p *Ntp) Offset() (time.Duration, error) {
 		total += offset
 	}
 
-	// cache the value to be reused
-	p.offset = total / time.Duration(len(offsets))
-	p.updatedAt = p.now()
-
-	return p.offset, nil
+	return total / time.Duration(len(offsets)), nil
 }

pkg/metrics/ntp_test.go

@@ -85,7 +85,7 @@ func TestNewNtp_Timeout(t *testing.T) {
 	}
 }
 
-func TestOffset_Cache(t *testing.T) {
+func TestOffset_Interval(t *testing.T) {
 	testCases := []struct {
 		name              string
 		interval          uint
@@ -113,12 +113,11 @@ func TestOffset_Cache(t *testing.T) {
 			expectedUpdatedAt: mustParse("2006-01-02 15:04:05", "2022-09-28 16:02:45"), // same as now
 		},
 		{
-			name:              "request before interval should use cached value",
+			name:              "request before interval should return interval error",
 			now:               nowMock("2022-09-28 16:02:45"),
 			updatedAt:         mustParse("2006-01-02 15:04:05", "2022-09-28 15:52:45"),
 			pool:              []string{"one", "two"},
-			offset:            20 * time.Millisecond,
-			expectedOffset:    20 * time.Millisecond,
+			expectedError:     ErrNotInInterval,
 			expectedUpdatedAt: mustParse("2006-01-02 15:04:05", "2022-09-28 15:52:45"),
 		},
 		{
@@ -131,17 +130,25 @@ func TestOffset_Cache(t *testing.T) {
 			expectedOffset:    30 * time.Millisecond,
 			expectedUpdatedAt: mustParse("2006-01-02 15:04:05", "2022-09-28 16:02:45"), // same as now
 		},
+		{
+			name:              "request with query error should update updatedAt",
+			now:               nowMock("2022-09-28 16:02:45"),
+			updatedAt:         mustParse("2006-01-02 15:04:05", "2022-09-28 15:42:45"),
+			pool:              []string{"one"},
+			ntpResponses:      []ntpResp{{nil, errors.New("this is an error")}},
+			expectedError:     ErrGettingNtpOffset,
+			expectedUpdatedAt: mustParse("2006-01-02 15:04:05", "2022-09-28 16:02:45"), // same as now
+		},
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
 			ntpMonitor := NewNtp(testCase.pool, testCase.timeout, testCase.interval)
 			ntpMonitor.ntpQuery = ntpQueryMock(testCase.ntpResponses...)
 			ntpMonitor.now = testCase.now
 			ntpMonitor.updatedAt = testCase.updatedAt
-			ntpMonitor.offset = testCase.offset
 			offset, err := ntpMonitor.Offset()
 			assert.Equal(t, testCase.expectedOffset, offset)
-			assert.Equal(t, testCase.expectedError, err)
+			assert.ErrorIs(t, err, testCase.expectedError)
 			assert.Equal(t, testCase.expectedUpdatedAt, ntpMonitor.updatedAt)
 		})
 	}