Address review comments

webapp/backend/pkg/database/interface.go

@@ -21,7 +21,7 @@ type DeviceRepo interface {
 	DeleteDevice(ctx context.Context, wwn string) error
 
 	SaveSmartAttributes(ctx context.Context, wwn string, collectorSmartData collector.SmartInfo) (measurements.Smart, error)
-	GetSmartAttributeHistory(ctx context.Context, wwn string, durationKey string, n int, offset int, attributes []string) ([]measurements.Smart, error)
+	GetSmartAttributeHistory(ctx context.Context, wwn string, durationKey string, selectEntries int, selectEntriesOffset int, attributes []string) ([]measurements.Smart, error)
 
 	SaveSmartTemperature(ctx context.Context, wwn string, deviceProtocol string, collectorSmartData collector.SmartInfo) error
 

webapp/backend/pkg/database/scrutiny_repository_device_smart_attributes.go

@@ -31,14 +31,17 @@ func (sr *scrutinyRepository) SaveSmartAttributes(ctx context.Context, wwn strin
 }
 
 // GetSmartAttributeHistory MUST return in sorted order, where newest entries are at the beginning of the list, and oldest are at the end.
-func (sr *scrutinyRepository) GetSmartAttributeHistory(ctx context.Context, wwn string, durationKey string, n int, offset int, attributes []string) ([]measurements.Smart, error) {
+// When selectEntries is > 0, only the most recent selectEntries database entries are returned, starting from the selectEntriesOffset entry.
+// For example, with selectEntries = 5, selectEntries = 0, the most recent 5 are returned. With selectEntries = 3, selectEntries = 2, entries
+// 2 to 4 are returned (2 being the third newest, since it is zero-indexed)
+func (sr *scrutinyRepository) GetSmartAttributeHistory(ctx context.Context, wwn string, durationKey string, selectEntries int, selectEntriesOffset int, attributes []string) ([]measurements.Smart, error) {
 	// Get SMartResults from InfluxDB
 
 	//TODO: change the filter startrange to a real number.
 
 	// Get parser flux query result
 	//appConfig.GetString("web.influxdb.bucket")
-	queryStr := sr.aggregateSmartAttributesQuery(wwn, durationKey, n, offset, attributes)
+	queryStr := sr.aggregateSmartAttributesQuery(wwn, durationKey, selectEntries, selectEntriesOffset, attributes)
 	log.Infoln(queryStr)
 
 	smartResults := []measurements.Smart{}
@@ -97,7 +100,7 @@ func (sr *scrutinyRepository) saveDatapoint(influxWriteApi api.WriteAPIBlocking,
 	return influxWriteApi.WritePoint(ctx, p)
 }
 
-func (sr *scrutinyRepository) aggregateSmartAttributesQuery(wwn string, durationKey string, n int, offset int, attributes []string) string {
+func (sr *scrutinyRepository) aggregateSmartAttributesQuery(wwn string, durationKey string, selectEntries int, selectEntriesOffset int, attributes []string) string {
 
 	/*
 
@@ -147,7 +150,7 @@ func (sr *scrutinyRepository) aggregateSmartAttributesQuery(wwn string, duration
 	if len(nestedDurationKeys) == 1 {
 		//there's only one bucket being queried, no need to union, just aggregate the dataset and return
 		partialQueryStr = append(partialQueryStr, []string{
-			sr.generateSmartAttributesSubquery(wwn, nestedDurationKeys[0], n, offset, attributes),
+			sr.generateSmartAttributesSubquery(wwn, nestedDurationKeys[0], selectEntries, selectEntriesOffset, attributes),
 			fmt.Sprintf(`%sData`, nestedDurationKeys[0]),
 			`|> sort(columns: ["_time"], desc: true)`,
 			`|> yield()`,
@@ -159,10 +162,10 @@ func (sr *scrutinyRepository) aggregateSmartAttributesQuery(wwn string, duration
 	subQueryNames := []string{}
 	for _, nestedDurationKey := range nestedDurationKeys {
 		subQueryNames = append(subQueryNames, fmt.Sprintf(`%sData`, nestedDurationKey))
-		if n > 0 {
+		if selectEntries > 0 {
 			// We only need the last `n + offset` # of entries from each table to guarantee we can
 			// get the last `n` # of entries starting from `offset` of the union
-			subQueries = append(subQueries, sr.generateSmartAttributesSubquery(wwn, nestedDurationKey, n+offset, 0, attributes))
+			subQueries = append(subQueries, sr.generateSmartAttributesSubquery(wwn, nestedDurationKey, selectEntries+selectEntriesOffset, 0, attributes))
 		} else {
 			subQueries = append(subQueries, sr.generateSmartAttributesSubquery(wwn, nestedDurationKey, 0, 0, attributes))
 		}
@@ -173,15 +176,15 @@ func (sr *scrutinyRepository) aggregateSmartAttributesQuery(wwn string, duration
 		`|> group()`,
 		`|> sort(columns: ["_time"], desc: true)`,
 	}...)
-	if n > 0 {
-		partialQueryStr = append(partialQueryStr, fmt.Sprintf(`|> tail(n: %d, offset: %d)`, n, offset))
+	if selectEntries > 0 {
+		partialQueryStr = append(partialQueryStr, fmt.Sprintf(`|> tail(n: %d, offset: %d)`, selectEntries, selectEntriesOffset))
 	}
 	partialQueryStr = append(partialQueryStr, `|> yield(name: "last")`)
 
 	return strings.Join(partialQueryStr, "\n")
 }
 
-func (sr *scrutinyRepository) generateSmartAttributesSubquery(wwn string, durationKey string, n int, offset int, attributes []string) string {
+func (sr *scrutinyRepository) generateSmartAttributesSubquery(wwn string, durationKey string, selectEntries int, selectEntriesOffset int, attributes []string) string {
 	bucketName := sr.lookupBucketName(durationKey)
 	durationRange := sr.lookupDuration(durationKey)
 
@@ -191,8 +194,8 @@ func (sr *scrutinyRepository) generateSmartAttributesSubquery(wwn string, durati
 		`|> filter(fn: (r) => r["_measurement"] == "smart" )`,
 		fmt.Sprintf(`|> filter(fn: (r) => r["device_wwn"] == "%s" )`, wwn),
 	}
-	if n > 0 {
-		partialQueryStr = append(partialQueryStr, fmt.Sprintf(`|> tail(n: %d, offset: %d)`, n, offset))
+	if selectEntries > 0 {
+		partialQueryStr = append(partialQueryStr, fmt.Sprintf(`|> tail(n: %d, offset: %d)`, selectEntries, selectEntriesOffset))
 	}
 	partialQueryStr = append(partialQueryStr, "|> schema.fieldsAsCols()")
 

webapp/backend/pkg/notify/notify.go

@@ -32,7 +32,7 @@ const NotifyFailureTypeSmartFailure = "SmartFailure"
 const NotifyFailureTypeScrutinyFailure = "ScrutinyFailure"
 
 // ShouldNotify check if the error Message should be filtered (level mismatch or filtered_attributes)
-func ShouldNotify(device models.Device, smartAttrs measurements.Smart, statusThreshold pkg.MetricsStatusThreshold, statusFilterAttributes pkg.MetricsStatusFilterAttributes, repeatNotifications bool, c *gin.Context, deviceRepo database.DeviceRepo) bool {
+func ShouldNotify(logger logrus.FieldLogger, device models.Device, smartAttrs measurements.Smart, statusThreshold pkg.MetricsStatusThreshold, statusFilterAttributes pkg.MetricsStatusFilterAttributes, repeatNotifications bool, c *gin.Context, deviceRepo database.DeviceRepo) bool {
 	// 1. check if the device is healthy
 	if device.DeviceStatus == pkg.DeviceStatusPassed {
 		return false
@@ -62,12 +62,16 @@ func ShouldNotify(device models.Device, smartAttrs measurements.Smart, statusThr
 	}
 
 	var failingAttributes []string
+	// Loop through the attributes to find the failing ones
 	for attrId, attrData := range smartAttrs.Attributes {
 		var status pkg.AttributeStatus = attrData.GetStatus()
+		// Skip over passing attributes
 		if status == pkg.AttributeStatusPassed {
 			continue
 		}
 
+		// If the user only wants to consider critical attributes, we have to check
+		// if the not-passing attribute is critical or not
 		if statusFilterAttributes == pkg.MetricsStatusFilterAttributesCritical {
 			critical := false
 			if device.IsScsi() {
@@ -82,34 +86,38 @@ func ShouldNotify(device models.Device, smartAttrs measurements.Smart, statusThr
 				}
 				critical = thresholds.AtaMetadata[attrIdInt].Critical
 			}
+			// Skip non-critical, non-passing attributes when this setting is on
 			if !critical {
 				continue
 			}
 		}
 
+		// Record any attribute that doesn't get skipped by the above two checks
 		failingAttributes = append(failingAttributes, attrId)
 	}
 
+	// If the user doesn't want repeated notifications when the failing value doesn't change, we need to get the last value from the db
+	var lastPoints []measurements.Smart
+	var err error
 	if !repeatNotifications {
-		lastPoints, err := deviceRepo.GetSmartAttributeHistory(c, c.Param("wwn"), database.DURATION_KEY_FOREVER, 1, 1, failingAttributes)
-		if err == nil && len(lastPoints) > 1 {
-			for _, attrId := range failingAttributes {
-				if lastPoints[0].Attributes[attrId].GetTransformedValue() != smartAttrs.Attributes[attrId].GetTransformedValue() {
-					return true
-				}
-			}
-			return false
+		lastPoints, err = deviceRepo.GetSmartAttributeHistory(c, c.Param("wwn"), database.DURATION_KEY_FOREVER, 1, 1, failingAttributes)
+		if err == nil || len(lastPoints) < 1 {
+			logger.Warningln("Could not get the most recent data points from the database. This is expected to happen only if this is the very first submission of data for the device.")
 		}
-		return true
-	} else {
-		for _, attr := range failingAttributes {
-			attrStatus := smartAttrs.Attributes[attr].GetStatus()
-			if pkg.AttributeStatusHas(attrStatus, requiredAttrStatus) {
+	}
+	for _, attrId := range failingAttributes {
+		attrStatus := smartAttrs.Attributes[attrId].GetStatus()
+		if pkg.AttributeStatusHas(attrStatus, requiredAttrStatus) {
+			if repeatNotifications {
+				return true
+			}
+			// This is checked again here to avoid repeating the entire for loop in the check above.
+			// Probably unnoticeably worse performance, but cleaner code.
+			if err != nil || len(lastPoints) < 1 || lastPoints[0].Attributes[attrId].GetTransformedValue() != smartAttrs.Attributes[attrId].GetTransformedValue() {
 				return true
 			}
 		}
 	}
-
 	return false
 }
 

webapp/backend/pkg/web/handler/upload_device_metrics.go

@@ -70,6 +70,7 @@ func UploadDeviceMetrics(c *gin.Context) {
 
 	//check for error
 	if notify.ShouldNotify(
+		logger,
 		updatedDevice,
 		smartData,
 		pkg.MetricsStatusThreshold(appConfig.GetInt(fmt.Sprintf("%s.metrics.status_threshold", config.DB_USER_SETTINGS_SUBKEY))),