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.

Sign up for GitHub

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

Jump to bottom

Return the cached value if it's not time to scan again yet #18

Merged
merged 2 commits into from
Aug 5, 2022
Merged

Return the cached value if it's not time to scan again yet #18

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2129f9
Return the cached value if it's not time to scan again yet
antifuchs Feb 11, 2021
2b711b6
Address review feedback: Shorten the time-to-scan logic
antifuchs Oct 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions readjson.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,17 @@ func readData(device string) (gjson.Result, error) {
timeToScan := false
if cacheOk {
timeToScan = time.Now().After(cacheValue.LastCollect.Add(options.SMARTctl.CollectPeriodDuration))
} else {
timeToScan = true
}

if timeToScan {
if timeToScan || !cacheOk {
antifuchs marked this conversation as resolved.
Show resolved Hide resolved
json, ok := readSMARTctl(device)
if ok {
jsonCache[device] = JSONCache{JSON: json, LastCollect: time.Now()}
return jsonCache[device].JSON, nil
}
return gjson.Parse("{}"), fmt.Errorf("smartctl returned bad data for device %s", device)
}
return gjson.Parse("{}"), fmt.Errorf("Too early collect called for device %s", device)
return cacheValue.JSON, nil
Copy link

Choose a reason for hiding this comment

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

It seems like originally not assumed to return already existent value from cache. Instead it supposed to return nothing and to me it seems more logical than return duplicated values.

Copy link
Contributor

@lahwaacz lahwaacz Oct 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So if the polling interval in smartctl_exporter is e.g. 300s and prometheus scrapes it more often, or if there are multiple scrapers, only the first time a meaningful value is returned and everything next gets nothing until the 300s pass? That does not sound right either.

Copy link

Choose a reason for hiding this comment

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

Also agree. Initially I was thinking that it should be managed by Prometheus only to reduce configurations. But at the end didn't want to be too rude and remove it completely. If the owner added it, perhaps he had some reasons?😁
I'm simply using 0s in my config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, removing caching may be the easiest / best thing to fix up this exporter.

}
return gjson.Parse("{}"), fmt.Errorf("Device %s unavialable", device)
}
Expand Down