Skip to content

feat: add simple retries for status requests #64

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

Closed
pboros wants to merge 1 commit into from
Closed

feat: add simple retries for status requests #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 28 additions & 8 deletions db/conn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package db
import (
"bufio"
"encoding/json"
"fmt"
"os"
"regexp"
"strconv"
Expand Down Expand Up @@ -100,7 +101,18 @@ func getFdb() fdb.Database {
return db
}

func GetStatus() (*models.FullStatus, error) {
func IsStatusIncomplete(status *models.FullStatus) bool {
if status.Cluster == nil ||
status.Cluster.DatabaseLockState == nil ||
status.Cluster.FaultTolerance == nil ||
status.Cluster.Data == nil ||
status.Cluster.DatabaseAvailable == false {
return true
}
return false
}

func getStatusOnce() (*models.FullStatus, error) {
start := time.Now()
conn := getFdb()
var status models.FullStatus
Expand All @@ -122,11 +134,7 @@ func GetStatus() (*models.FullStatus, error) {
}

if os.Getenv("DEBUG_LOG_INCOMPLETE_STATUS") == "true" {
if status.Cluster == nil ||
status.Cluster.DatabaseLockState == nil ||
status.Cluster.FaultTolerance == nil ||
status.Cluster.Data == nil ||
status.Cluster.DatabaseAvailable == false {
if IsStatusIncomplete(&status) {
statusString := string(statusJson.([]byte))
log.Debug().Str("status_json", statusString).Msg("status json is missing cluster fields")

Expand All @@ -135,13 +143,25 @@ func GetStatus() (*models.FullStatus, error) {
Dur("took", d).
Str("status_json", statusString).
Msg("status json is missing cluster fields")

return nil, fmt.Errorf("incomplete status")

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

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

This code path returns an error when DEBUG_LOG_INCOMPLETE_STATUS is enabled and the status is incomplete, but does not return an error when DEBUG_LOG_INCOMPLETE_STATUS is disabled. This creates inconsistent behavior where incomplete status is only treated as an error in debug mode. The error return should be moved outside the debug flag check to ensure incomplete status is always treated as an error, or the function should explicitly return the incomplete status without error when not in debug mode.

Copilot uses AI. Check for mistakes.

@greptile-apps greptile-apps Bot Jan 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

logic: returning error only when DEBUG_LOG_INCOMPLETE_STATUS=true creates inconsistent behavior

When the env var is not set, incomplete status is returned as success. When set, it triggers retry. This means production (without the debug flag) silently accepts incomplete status while debug mode rejects it.

Consider either:

  • Always returning error for incomplete status (and remove the env var check)
  • Or never returning error (just log it)

The current behavior makes debugging harder since enabling the debug flag changes the functional behavior, not just logging. Is the intent to eventually always reject incomplete status, or is this env var a temporary debugging aid that should only affect logging?

Prompt To Fix With AI 
This is a comment left during a code review.
Path: db/conn.go
Line: 146:146

Comment:
**logic:** returning error only when `DEBUG_LOG_INCOMPLETE_STATUS=true` creates inconsistent behavior

When the env var is not set, incomplete status is returned as success. When set, it triggers retry. This means production (without the debug flag) silently accepts incomplete status while debug mode rejects it.

Consider either:
- Always returning error for incomplete status (and remove the env var check)
- Or never returning error (just log it)

The current behavior makes debugging harder since enabling the debug flag changes the functional behavior, not just logging. Is the intent to eventually always reject incomplete status, or is this env var a temporary debugging aid that should only affect logging?

How can I resolve this? If you propose a fix, please make it concise.

}
}

return &status, nil
}

func GetStatus() (*models.FullStatus, error) {
status, err := getStatusOnce()
if err != nil {
log.Error().Err(err).Msg("failed to get status, retrying")
time.Sleep(1 * time.Second)
status, err = getStatusOnce()
if err != nil {
log.Error().Err(err).Msg("failed to get status, retry failed")
}
}
return status, err
}

func isTLSmode(c *string) bool {
// Find if must run in TLS mode
file, err := os.Open(*c)
Expand Down
46 changes: 35 additions & 11 deletions metrics/metric_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
package metrics

import (
"context"
"net/http"
"os"
"sync"
"time"

ulog "github.com/tigrisdata/fdb-exporter/util/log"
Expand Down Expand Up @@ -56,24 +58,46 @@ func (m *MetricProvider) Close() {

// Periodic data collection, called from main in a goroutine
func (mp *MetricProvider) Collect() {

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

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

The interval change from 10 seconds to 3 seconds is undocumented. Consider adding a comment explaining why this interval was reduced, especially since the original TODO suggested making it configurable.

Suggested change
func (mp *MetricProvider) Collect() {
func (mp *MetricProvider) Collect() {
// NOTE: interval was reduced from 10s to 3s to improve metric freshness for dashboards
// and alerts while keeping collection overhead acceptable. This value may still be
// made configurable in the future if more tuning is required.

Copilot uses AI. Check for mistakes.
// TODO make this configurable
interval := 10 * time.Second
var reporterSwap sync.Mutex

interval := 3 * time.Second
ticker := time.NewTicker(interval)
defer ticker.Stop()

if err := mp.reporter.collectOnce(); err != nil {
ulog.E(err, "failed to collect metrics")
}

for range ticker.C {
newReporter := NewMetricReporter()
if err := newReporter.collectOnce(); err != nil {
ulog.E(err, "failed to collect metrics in a tick")
}
time.Sleep(1 * time.Second) // Wait a bit before serving new tally's data (otherwise the first query will return 0)
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)

@greptile-apps greptile-apps Bot Jan 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

logic: timeout (2s) shorter than the sleep inside goroutine (1s at line 82)

If collectOnce() takes >1s, the goroutine will always timeout since it sleeps for 1s after collection. This defeats the purpose of the timeout. Consider using a timeout of at least 2.5-3s, or removing the 1s sleep and handling reporter swapping differently.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: metrics/metric_provider.go
Line: 69:69

Comment:
**logic:** timeout (2s) shorter than the sleep inside goroutine (1s at line 82)

If `collectOnce()` takes >1s, the goroutine will always timeout since it sleeps for 1s after collection. This defeats the purpose of the timeout. Consider using a timeout of at least 2.5-3s, or removing the 1s sleep and handling reporter swapping differently.

How can I resolve this? If you propose a fix, please make it concise.


done := make(chan struct{})

go func() {
defer close(done)

oldReporter := mp.reporter
mp.reporter = newReporter
newReporter := NewMetricReporter()
if err := newReporter.collectOnce(); err != nil {
ulog.E(err, "failed to collect metrics in a tick")
return
Comment on lines +81 to +82

@greptile-apps greptile-apps Bot Jan 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

logic: return on error leaks the goroutine

When collectOnce() fails, the goroutine returns early without closing done channel or swapping reporters. Meanwhile, the timeout still fires after 2s, and the cancel() is still called. The old reporter continues to be used, and errors will repeat on every tick.

Suggested change
ulog.E(err, "failed to collect metrics in a tick")
return
if err := newReporter.collectOnce(); err != nil {
ulog.E(err, "failed to collect metrics in a tick")
newReporter.Close()
return
}

Close the new reporter on error to avoid leaking resources.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: metrics/metric_provider.go
Line: 78:79

Comment:
**logic:** `return` on error leaks the goroutine

When `collectOnce()` fails, the goroutine returns early without closing `done` channel or swapping reporters. Meanwhile, the timeout still fires after 2s, and the `cancel()` is still called. The old reporter continues to be used, and errors will repeat on every tick.

```suggestion
			if err := newReporter.collectOnce(); err != nil {
				ulog.E(err, "failed to collect metrics in a tick")
				newReporter.Close()
				return
			}
```

Close the new reporter on error to avoid leaking resources.

How can I resolve this? If you propose a fix, please make it concise.

}

oldReporter.Close()
time.Sleep(1 * time.Second)

reporterSwap.Lock()
oldReporter := mp.reporter
mp.reporter = newReporter
Comment on lines +88 to +89

@greptile-apps greptile-apps Bot Jan 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

logic: race condition: mp.reporter can be read by HTTP requests in ServeHTTP while being written here

The goroutine updates mp.reporter without synchronization. If an HTTP request calls ServeHTTP (line 49) at the same time, it reads mp.reporter.reporter.HTTPHandler() leading to a data race.

Suggested change
oldReporter := mp.reporter
mp.reporter = newReporter
mp.mu.Lock()
oldReporter := mp.reporter
mp.reporter = newReporter
mp.mu.Unlock()

Add a sync.RWMutex field to MetricProvider and protect both reads (in ServeHTTP) and writes (here). Is there a reason you avoided using a mutex here, or was this an oversight?

Prompt To Fix With AI 
This is a comment left during a code review.
Path: metrics/metric_provider.go
Line: 84:85

Comment:
**logic:** race condition: `mp.reporter` can be read by HTTP requests in `ServeHTTP` while being written here

The goroutine updates `mp.reporter` without synchronization. If an HTTP request calls `ServeHTTP` (line 49) at the same time, it reads `mp.reporter.reporter.HTTPHandler()` leading to a data race.

```suggestion
			mp.mu.Lock()
			oldReporter := mp.reporter
			mp.reporter = newReporter
			mp.mu.Unlock()
```

Add a `sync.RWMutex` field to `MetricProvider` and protect both reads (in `ServeHTTP`) and writes (here). Is there a reason you avoided using a mutex here, or was this an oversight?

How can I resolve this? If you propose a fix, please make it concise.

oldReporter.Close()
reporterSwap.Unlock()
}()

select {
case <-done:
// finished within timeout
case <-ctx.Done():
ulog.E(ctx.Err(), "metric collection tick timed out")
}

cancel()

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

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

The context cancel function should be called immediately after the timeout context is created using defer to ensure cleanup happens regardless of the code path taken. Move defer cancel() to line 70, right after the context creation.

Copilot uses AI. Check for mistakes.
}
defer ticker.Stop()
}