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.

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

Sabnzbd fix #246

Closed
wants to merge 2 commits into from
Closed

Sabnzbd fix #246

wants to merge 2 commits into from

Conversation

Crosenhain
Copy link

Description of the change
Two things:

  • I changed the API request path to remove "/sabnzbd" since we set that (or not!) using the URL environment variable
  • I modified model.go to handle cases where the map is empty, which would throw a 'runtime error: index out of range [-1]' error

Benefits

The sabnzbd exporter works more reliably, and in more configurations.

Possible drawbacks

There is a possibility the change to the sabnzbd API request path could affect how others have configured their exporter.

Applicable issues

  • fixes #

Additional information

@@ -166,6 +166,10 @@ func (q *QueueStats) UnmarshalJSON(data []byte) error {

// latestStat gets the most recent date's value from a map of dates to values
func latestStat(m map[string]int) (string, int) {
if len(m) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What situation was leading to the map being empty?

I'm a little worried this may actually cause a bug that leads to incorrect metrics. In the collector itself, we compare the cached day string to the one returned from the new Stats object, and if they're not the same, we assume the day has rolled over, and reset the counter:

func (s serverStatCache) Update(stat model.ServerStat) ServerStats {
s.total = stat.Total
if stat.DayParsed != s.todayKey {
s.articlesTriedHistorical += s.articlesTriedToday
s.articlesSuccessHistorical += s.articlesSuccessToday
s.articlesTriedToday = 0
s.articlesSuccessToday = 0
s.todayKey = stat.DayParsed
}
s.articlesTriedToday = stat.ArticlesTried
s.articlesSuccessToday = stat.ArticlesSuccess
return s
}

So if we receive an empty map due to, for example, a sqlite error on the sab side, we could assume the day has changed too early, resulting in double counting of server stats.

I think we can verify behavior here with a quick unit test that:

  1. Creates a new statCache
  2. calls Update with a non-zero serverStats object
  3. calls Update with a zeroed serverStats object
  4. calls Update one more time with the object from step 2.

And then verify that the count is correct.

@@ -166,6 +166,10 @@ func (q *QueueStats) UnmarshalJSON(data []byte) error {

// latestStat gets the most recent date's value from a map of dates to values
func latestStat(m map[string]int) (string, int) {
if len(m) == 0 {
// Return a zero-value or suitable default
Copy link
Collaborator

Choose a reason for hiding this comment

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

The indentation here is what's causing the linter to barf. if you just run go fmt it should fix it up.

@@ -189,7 +189,7 @@ func NewSabnzbdCollector(config *config.SabnzbdConfig) (*SabnzbdCollector, error
func (s *SabnzbdCollector) doRequest(mode string, target interface{}) error {
params := client.QueryParams{}
params.Add("mode", mode)
return s.client.DoRequest("/sabnzbd/api", target, params)
return s.client.DoRequest("/api", target, params)
Copy link
Collaborator

Choose a reason for hiding this comment

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

While I understand the thought process here, this would be a breaking change for existing users :-/ Is this actively breaking your setup? If not, we can create an issue and make this change when we do a major version bump. If it is breaking your setup, we probably want to put this behind a flag for now.

Choose a reason for hiding this comment

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

This is actively breaking my setup. Can we introduce a defaulted environment variable here?

Copy link
Owner

Choose a reason for hiding this comment

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

@rtrox I am ok with this breaking change going into a minor release.

@rtrox
Copy link
Collaborator

rtrox commented Dec 20, 2023

Summing, it looks like there are a couple changes needed on this PR to land:

  1. We need a unittest to verify the empty map handling doesn't break stats. I believe in the interim, an empty map should just result in a 500 until there are stats ready to be returned. While that isn't ideal, I want to make sure the fix maintains the integrity of the stats
  2. run go fmt to fix the linter

otherwise, it looks like we're g2g

@Crosenhain
Copy link
Author

Sounds good, I've got some time now away from work so I'll have a go at the updates.

@rtrox
Copy link
Collaborator

rtrox commented Jan 25, 2024

The sab fix portion of this PR should be resolved as of #252 -- going to close this PR in favor of that one, but please feel free to re-open/open new for the api_url change

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.

4 participants