From 01c5a7fdfe7191569afcee3f0dc01ffe9f9881f6 Mon Sep 17 00:00:00 2001 From: Aram Akhavan Date: Fri, 8 Dec 2023 11:18:13 -0800 Subject: [PATCH] Address review comments --- webapp/backend/pkg/database/interface.go | 2 +- ...tiny_repository_device_smart_attributes.go | 25 ++++++------ webapp/backend/pkg/notify/notify.go | 38 +++++++++++-------- .../pkg/web/handler/upload_device_metrics.go | 1 + 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/webapp/backend/pkg/database/interface.go b/webapp/backend/pkg/database/interface.go index e72d2a9..2b34a1f 100644 --- a/webapp/backend/pkg/database/interface.go +++ b/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 diff --git a/webapp/backend/pkg/database/scrutiny_repository_device_smart_attributes.go b/webapp/backend/pkg/database/scrutiny_repository_device_smart_attributes.go index 7a8a194..b1abf6c 100644 --- a/webapp/backend/pkg/database/scrutiny_repository_device_smart_attributes.go +++ b/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()") diff --git a/webapp/backend/pkg/notify/notify.go b/webapp/backend/pkg/notify/notify.go index aac6cad..bf927ee 100644 --- a/webapp/backend/pkg/notify/notify.go +++ b/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 } diff --git a/webapp/backend/pkg/web/handler/upload_device_metrics.go b/webapp/backend/pkg/web/handler/upload_device_metrics.go index f83107f..0814433 100644 --- a/webapp/backend/pkg/web/handler/upload_device_metrics.go +++ b/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))),